public inbox for [email protected]
help / color / mirror / Atom feedFrom: shveta malik <[email protected]>
To: SATYANARAYANA NARLAPURAM <[email protected]>
Cc: vignesh C <[email protected]>
Cc: Fujii Masao <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: shveta malik <[email protected]>
Subject: Re: [PATCH] Release replication slot on error in SQL-callable slot functions
Date: Wed, 27 May 2026 09:23:54 +0530
Message-ID: <CAJpy0uBo-OuzyZZ=LY8L48Udt8=1Mffh2rrpXaTbKK_F9sJx7A@mail.gmail.com> (raw)
In-Reply-To: <CAHg+QDdEGGQrmQXwH0_Y+DRe_tx5jxv9K+7vpCnooQn2_6QykA@mail.gmail.com>
References: <CAHg+QDeuf9tCq3ce=kgFMJP0m=PZC+wi6B=yS+7V0vNXjLS31w@mail.gmail.com>
<CAHGQGwFZaWj8DctXuhWQZwSqi631=NKzQJyDV4yqT1Qapt8MFQ@mail.gmail.com>
<CALDaNm1Jjun=by60V-4EpLZe4pAKy0qVZ7ptyHGVCuDyKfo2xQ@mail.gmail.com>
<CAHg+QDcu2x0mjkBSqRxP_8EQ6UmpuX_jMgdKLDkAL1=N6wzZCQ@mail.gmail.com>
<CAJpy0uCmW_NUZN8mw26onvfoFzH_oMrFSKhLUhz896nDgf8c7Q@mail.gmail.com>
<CAHg+QDcf_9prAX=TaSO3UUiCLVD53bEw-KLqzAEXi+ud7h+Z4w@mail.gmail.com>
<CAJpy0uBShUF_xm0=BVWivpWHt-4zs__k_3wL1RRjpi0Av8nsog@mail.gmail.com>
<CAHg+QDf5PVyFgesBNs1GvOnuk_khoXifo96A7QW1EJ8zhhBxyw@mail.gmail.com>
<CAJpy0uCCqFLY7pu0RQVcS9fRr0FimFMuHPsMBQ-KzEGX3BEGPA@mail.gmail.com>
<CAJpy0uDHMvpUAdwXA3X7ugmO8S7kry-ZtrKUcugpX3WWp8hykw@mail.gmail.com>
<CAHg+QDdEGGQrmQXwH0_Y+DRe_tx5jxv9K+7vpCnooQn2_6QykA@mail.gmail.com>
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
Attachments:
[text/plain] 0001-top-up-changes.patch.txt (2.8K, 2-0001-top-up-changes.patch.txt)
download | inline diff:
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
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [PATCH] Release replication slot on error in SQL-callable slot functions
In-Reply-To: <CAJpy0uBo-OuzyZZ=LY8L48Udt8=1Mffh2rrpXaTbKK_F9sJx7A@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox