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. Thanks, Satya
v3-0001-Release-replication-slot-on-error-in-slot-SQL-functions.patch
Description: Binary data
