Hi, Thanks again for reviewing the test cases. Please find my comments inline below:
On Wed, Mar 18, 2026 at 12:01 PM shveta malik <[email protected]> wrote: > > > Few comments: > > 1) > + > +# Physical replication slots for the two standbys > +$primary->safe_psql('postgres', > + "SELECT pg_create_physical_replication_slot('sb1_slot');"); > +$primary->safe_psql('postgres', > + "SELECT pg_create_physical_replication_slot('sb2_slot');"); > +$primary->safe_psql('postgres', > + "SELECT pg_create_physical_replication_slot('first_slot');"); > > Can we please write a comment atop 'first_slot' creation for its purpose. > The comment '+# Physical replication slots for the two standbys' > confuses as we are creating 3 slots following that comment. > Moved this to PART E (the test-case where this slot is being used). This is also suggested in one of your comments below. > 2) > +# PART A: Plain list (ALL mode) blocks when any slot is unavailable > > +# PART C: Verify plain list (ALL mode) requires ALL slots > > I think we can merge the 2 tests. Is there a specific reason we have > it in 2 different sections (am I missing something?). > > Part A can first test blocking on inactive standby (which it is doing > already) and then restart standby and can check that logical decoding > now proceeds. Part C can then be removed. > I did not remove PART C entirely, however, I removed the portions that overlapped with PART A. I would suggest not just evaluating each test case independently, it would be useful to also consider the overall flow to understand why PART B follows PART A, and so on. Viewed sequentially, PART A tests a unique scenario while also setting up the environment needed for PART B (quorum mode). Similarly, PART C covers a different scenario and prepares the setup for the test case in PART D. This approach helps avoid unnecessary stop/start cycles of the synchronous standby. > 3) > Same with these 2: > +# PART B: ANY mode (quorum) — logical decoding proceeds with N-of-M slots > > +# PART D: ANY mode — verify it works with either standby > > Part B tests that it works when standby1 is down. > Part D tests that it works when standby2 is down and also works when > both are up. > > I think we don't need these duplicate test cases (unless I am missing > something?), we can merge Part D to Part B itself i.e. test with > standby1 down and up in a single test. > Removed PART D. It was almost similar to PART B. > 4) > These 2 tests are basically testing the same thing. Do we need both? > +# FIRST 1 should skip sb1_slot (unavailable) and use sb2_slot. > > +# Test that FIRST 1 is different from plain list — FIRST 1 succeeds > with one down. > Yes, they are actually the same. Hence removed the latter one. > > 5) > I see that we only need 'first_slot' for the last test and do not have > even a standby associated with it. So we can even move it's creation > to that test itself instead of creating it in the beginning and adding > comments to explain 'why'. > This is related to comment 1 above. PFA patch that addresses above comments. -- With Regards, Ashutosh Sharma.
v20260318-0001-Add-FIRST-N-and-ANY-N-syntax-support-to-synchronized.patch
Description: Binary data
