pgcat icon indicating copy to clipboard operation
pgcat copied to clipboard

Add 'Fail' mode when no shard selected

Open nothrow opened this issue 1 year ago • 4 comments
trafficstars

Implementation for feature request & proposed solution in https://github.com/postgresml/pgcat/issues/858.

Adds new option for default_shard.

nothrow avatar Nov 11 '24 11:11 nothrow

I am fine with the change but I think there should be tests for this. We are already testing the default_shard parameter here, adding one test case for fail should be straight forward.

magec avatar Nov 12 '24 13:11 magec

@nothrow @magec I've implemented a test case for this:

diff --git a/tests/ruby/sharding_spec.rb b/tests/ruby/sharding_spec.rb
index 746627d..456997c 100644
--- a/tests/ruby/sharding_spec.rb
+++ b/tests/ruby/sharding_spec.rb
@@ -129,6 +129,26 @@ describe "Sharding" do
           end
         end
 
+        context "when no_shard_specified_behavior config is set to fail" do
+          let(:no_shard_specified_behavior) { "fail" }
+
+          context "with no shard comment" do
+            it "fails and doesn't send the query" do
+              conn = PG.connect(processes.pgcat.connection_string("sharded_db", "sharding_user"))
+              expect { conn.async_exec("SELECT 1 + 2").to_a }.to raise_error(PG::SystemError).with_message(/NoShardSelected/)
+            end
+          end
+
+          context "with a shard comment" do
+            it "honors the comment" do
+              conn = PG.connect(processes.pgcat.connection_string("sharded_db", "sharding_user"))
+              25.times { conn.async_exec("#{comment_to_use} SELECT 1 + 2") }
+
+              expect(processes.all_databases.map(&:count_select_1_plus_2)).to eq([0, 25, 0])
+            end
+          end
+        end
+
         context "when no_shard_specified_behavior config is set to random_healthy" do
           let(:no_shard_specified_behavior) { "random_healthy" }

Hopefully this can be merged soon :)

lc-guy avatar Mar 25 '25 11:03 lc-guy

I' fine with merging this. What do you think @drdrsh ?

magec avatar Mar 25 '25 11:03 magec

@drdrsh @magec would it be possible to take another look at this?

lc-guy avatar May 20 '25 11:05 lc-guy