On Tue, May 26, 2026 at 1:41 PM SATYANARAYANA NARLAPURAM <[email protected]> wrote: > > Hi, > > On Mon, May 25, 2026 at 10:50 PM shveta malik <[email protected]> wrote: >> >> On Tue, May 26, 2026 at 10:06 AM shveta malik <[email protected]> wrote: >> > >> > On Tue, May 26, 2026 at 12:31 AM SATYANARAYANA NARLAPURAM >> > <[email protected]> wrote: >> > > >> > > Hi, >> > > >> > > On Mon, May 25, 2026 at 2:58 AM shveta malik <[email protected]> >> > > wrote: >> > >> >> > >> On Mon, May 25, 2026 at 12:42 PM SATYANARAYANA NARLAPURAM >> > >> <[email protected]> wrote: >> > >> > >> > >> > Hi >> > >> > >> > >> > On Fri, May 22, 2026 at 2:16 AM shveta malik <[email protected]> >> > >> > wrote: >> > >> >> >> > >> >> Thanks for reporting the issue. I could reproduce the same issue with >> > >> >> all these as well: >> > >> >> >> > >> >> pg_logical_slot_peek_changes >> > >> >> pg_logical_slot_get_binary_changes >> > >> >> pg_logical_slot_peek_binary_changes >> > >> > >> > >> > >> > >> > Please find the attached v2 patch that addressed these three cases as >> > >> > well. >> > >> > >> > >> >> > >> Thank You for addressuing these cases. A few comments: >> > >> >> > >> 1) >> > >> >> > >> +-- Test 2: session remains usable after the error (MyReplicationSlot >> > >> cleared) >> > >> >> > >> It shoudl be part of 'Test 1' itself and thus should not be named as >> > >> 'Test 2' >> > >> >> > >> 2) >> > >> -------- >> > >> +-- Test 4: copy_replication_slot with max_replication_slots exceeded. >> > >> +-- We reduce max_replication_slots artificially by filling all >> > >> remaining slots. >> > >> +-- Instead, trigger an error by copying to an already-existing name. >> > >> +DO $$ >> > >> +BEGIN >> > >> + PERFORM pg_copy_logical_replication_slot('regression_slot_t3', >> > >> 'regression_slot_t3'); >> > >> +EXCEPTION WHEN OTHERS THEN >> > >> + RAISE NOTICE 'caught: %', SQLERRM; >> > >> +END; >> > >> +$$; >> > >> +-- The original slot must still exist and be usable >> > >> +SELECT count(*) = 1 AS orig_slot_ok FROM pg_replication_slots >> > >> + WHERE slot_name = 'regression_slot_t3'; >> > >> ----------- >> > >> >> > >> I don't think we can hit the Assert with above test (at-least I could >> > >> not). Since creation of slot itself will fail as the slot with >> > >> same-name already exists, MyReplicationSlot will never be set and thus >> > >> Assert will not be hit. A better testcase will be below which fails >> > >> during LoadOutputPlugin() after slot-creation and MyReplicationSlot is >> > >> set already. >> > >> >> > >> SELECT pg_create_logical_replication_slot('src_slot', 'test_decoding'); >> > >> >> > >> DO $$ >> > >> BEGIN >> > >> PERFORM pg_copy_logical_replication_slot('src_slot', 'dst_slot', >> > >> false, 'nonexistent_plugin'); >> > >> EXCEPTION WHEN others THEN >> > >> RAISE NOTICE 'caught: %', SQLERRM; >> > >> END $$; >> > >> >> > >> SELECT count(*) FROM pg_logical_slot_get_changes('src_slot', NULL, >> > >> NULL); >> > >> >> > >> 3) >> > >> So overall these are the problematic APIs: >> > >> >> > >> pg_create_logical_replication_slot >> > >> pg_replication_slot_advance >> > >> pg_copy_logical_replication_slot >> > >> pg_logical_slot_peek_binary_changes >> > >> pg_logical_slot_peek_changes >> > >> pg_logical_slot_get_changes >> > >> pg_logical_slot_get_binary_changes >> > >> >> > >> First 3 are are mutually exclusive fixes fow which we have added >> > >> testcases. Last 4 are addressed by fixing common function >> > >> pg_logical_slot_get_changes_guts(). I think we should add a test case >> > >> for at-least any one of these APIs to cover >> > >> pg_logical_slot_get_changes_guts(). >> > > >> > > >> > > Thanks for reviewing. Please review the attached v3 patch. >> > > >> > >> > A few trivial things: >> > >> > 1) >> > pg_replication_slot_advance: >> > + PG_TRY(); >> > + { >> > + /* Acquire the slot so we "own" it */ >> > + ReplicationSlotAcquire(NameStr(*slotname), true, true); >> > + /* A slot whose restart_lsn has never been reserved cannot be advanced */ >> > + if (!XLogRecPtrIsValid(MyReplicationSlot->data.restart_lsn)) >> > >> > >> > We can have a blank line after ReplicationSlotAcquire for better >> > readability. >> > >> > 2) >> > >> > +SELECT 'init' FROM >> > pg_create_logical_replication_slot('regression_slot_t3', >> > 'test_decoding', true); >> > +SELECT count(*) = 1 AS slot_exists FROM pg_replication_slots >> > + WHERE slot_name = 'regression_slot_t3'; >> > >> > The intent is not clear why are we checking existence of >> > regression_slot_t3? I think we can skip it (or else add a comment if >> > really needed). The success of previous >> > pg_create_logical_replication_slot is enough to confirm that session >> > is healthy to run other slot related queries. >> > >> > 3) >> > +SELECT pg_drop_replication_slot('regression_slot_phy'); >> > + >> > +-- cleanup >> > +SELECT pg_drop_replication_slot('regression_slot_t3'); >> > >> > We can move drop of 'regression_slot_phy' too under '-- cleanup' >> > >> > ~~ >> > >> > I have no further comments other than the trivial things mentioned above. >> > >> >> Missed to inform this earlier, I am not able to apply any version of >> the patches shared so far with 'git am'. It gives error, 'patch -p1' >> works. >> >> git am v3-0001-Release-replication-slot-on-error-in-slot-SQL-functions.patch >> Patch format detection failed. > > Thanks , Shveta! Please find the attached v4 patch that addressed your > comments. >
Thank You for the patch. I noticed that we are creating regression_slot_t3 as a a temporary slot, is that intentional? I think creating a permanent slot here will be a better testcase. I have made a few cosmetic changes for better readability along with creating the 'permanent' regression_slot_t3 slot. Please incorporate what you think is okay. I have no more comments. thanks Shveta
From fd4e8f731fbfa5ccef88338d6fa39fe2b2a1820b Mon Sep 17 00:00:00 2001 From: Shveta Malik <[email protected]> Date: Wed, 27 May 2026 09:18:50 +0530 Subject: [PATCH] top-up changes --- contrib/test_decoding/sql/slot.sql | 11 +++++++++-- src/backend/replication/slotfuncs.c | 3 +++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/contrib/test_decoding/sql/slot.sql b/contrib/test_decoding/sql/slot.sql index eabbe05cc90..d8e0adccbfb 100644 --- a/contrib/test_decoding/sql/slot.sql +++ b/contrib/test_decoding/sql/slot.sql @@ -206,14 +206,18 @@ EXCEPTION WHEN OTHERS THEN RAISE NOTICE 'caught: %', SQLERRM; END; $$; + +-- the concerned slot must not exist (it was dropped on error) SELECT count(*) = 0 AS slot_was_dropped FROM pg_replication_slots WHERE slot_name = 'regression_slot_error'; -SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot_t3', 'test_decoding', true); +-- the session is still usable +SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot_t3', 'test_decoding', false); -- pg_replication_slot_advance: error after acquiring the slot should -- release it so the session stays usable. SELECT slot_name FROM pg_replication_slot_advance('regression_slot_t3', pg_current_wal_lsn()); + DO $$ BEGIN PERFORM pg_replication_slot_advance('regression_slot_t3', '0/1'); @@ -221,7 +225,8 @@ EXCEPTION WHEN OTHERS THEN RAISE NOTICE 'caught expected error'; END; $$; --- the session is still healthy + +-- the session is still usable SELECT slot_name FROM pg_replication_slot_advance('regression_slot_t3', pg_current_wal_lsn()); -- pg_copy_logical_replication_slot: error after creating the destination @@ -233,9 +238,11 @@ EXCEPTION WHEN OTHERS THEN RAISE NOTICE 'caught: %', SQLERRM; END; $$; + -- the destination slot must not exist (it was dropped on error) SELECT count(*) = 0 AS dst_slot_dropped FROM pg_replication_slots WHERE slot_name = 'regression_slot_dst'; + -- the session is still usable SELECT count(*) >= 0 AS changes_ok FROM pg_logical_slot_get_changes('regression_slot_t3', NULL, NULL); diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c index 9ba9bd892e6..d32e9187b95 100644 --- a/src/backend/replication/slotfuncs.c +++ b/src/backend/replication/slotfuncs.c @@ -194,6 +194,7 @@ create_logical_replication_slot(char *name, char *plugin, */ if (MyReplicationSlot != NULL) ReplicationSlotDropAcquired(); + PG_RE_THROW(); } PG_END_TRY(); @@ -633,6 +634,7 @@ pg_replication_slot_advance(PG_FUNCTION_ARGS) { if (MyReplicationSlot != NULL) ReplicationSlotRelease(); + PG_RE_THROW(); } PG_END_TRY(); @@ -896,6 +898,7 @@ copy_replication_slot(FunctionCallInfo fcinfo, bool logical_slot) */ if (MyReplicationSlot != NULL) ReplicationSlotDropAcquired(); + PG_RE_THROW(); } PG_END_TRY(); -- 2.34.1
