Subject: Re: [PATCH] Release replication slot on error in SQL-callable slot functions

From: vignesh C <[email-masked]>
Date: Thu, 21 May 2026 12:19:42 +0530

On Mon, 11 May 2026 at 08:31, Fujii Masao <[email-masked]> wrote:
>
> On Sun, May 10, 2026 at 5:45 AM SATYANARAYANA NARLAPURAM
> <[email-masked]> wrote:
> >
> > Hi Hackers,
> >
> > SQL-callable replication slot functions acquire a slot (setting
> > the process-global MyReplicationSlot) but can then ERROR before reaching
> > ReplicationSlotRelease(). If such an error is caught by a PL/pgSQL
> > EXCEPTION block (which uses a subtransaction), MyReplicationSlot remains
> > set because there is no subtransaction-level cleanup hook for replication
> > slots.
> >
> > Any subsequent slot operation in the same session then hits
> > Assert(MyReplicationSlot == NULL) and crashes the backend on assert
> > enabled builds. In release builds the stale MyReplicationSlot is silently overwritten,
> > permanently orphaning the old slot as "active." The orphaned slot blocks any other
> > session from acquiring it, vacuum and WAL deletion.
> >
> > Repro:
> >
> > SELECT pg_create_logical_replication_slot('adv_test', 'test_decoding');
> >
> > DO $$ BEGIN
> >     PERFORM pg_replication_slot_advance('adv_test', '0/1'::pg_lsn);
> > EXCEPTION WHEN others THEN
> >     RAISE NOTICE 'caught: %', SQLERRM;
> > END $$;
> >
> > SELECT count(*) FROM pg_logical_slot_get_changes('adv_test', NULL, NULL);
> >
> > 2026-05-09 19:45:06.619 UTC [1096805] STATEMENT:  SELECT pg_create_logical_replication_slot('adv_test', 'test_decoding');
> > TRAP: failed Assert("MyReplicationSlot == NULL"), File: "slot.c", Line: 638, PID: 1096805
> >
> >
> > Attached a patch to address this by wrapping error-prone paths in PG_TRY/PG_CATCH blocks
> > and call ReplicationSlotRelease().
>
> Thanks for the report and the patch!
>
> I think wrapping the slot-processing code with PG_TRY()/PG_CATCH() seems
> a good direction for addressing the issue you reported.
>
>
> +   PG_CATCH();
> +   {
> +       ReplicationSlotRelease();
>
> When create_logical_replication_slot() is called with temporary = true,
> the created logical replication slot has RS_TEMPORARY persistency. Such a slot
> is not dropped by ReplicationSlotRelease(), whereas an RS_EPHEMERAL slot is
> dropped via ReplicationSlotDropAcquired().
>
> So even with the v1 patch, a temporary logical replication slot can remain
> unexpectedly if pg_create_logical_replication_slot() throws an error.
> In this case, should create_logical_replication_slot() explicitly drop the slot
> with ReplicationSlotDropAcquired(), or temporarily change the slot persistency
> to RS_EPHEMERAL before calling ReplicationSlotRelease()?
>
>
> Does a newly created logical replication slot created by
> pg_copy_logical_replication_slot() have the same issue?

Additionally pg_logical_slot_get_changes also has the same issue, it
can be reproduced by the following:
SELECT pg_create_logical_replication_slot('test_slot_1', 'test_decoding');

DO $$
BEGIN
    -- This will ERROR if the slot_get changes fails for the slot.
    PERFORM 1 FROM pg_logical_slot_get_changes('test_slot_1', NULL,
NULL, 'nonexistent-option', 'val');
EXCEPTION WHEN others THEN
    RAISE NOTICE 'caught: %', SQLERRM;
END $$;

SELECT count(*) FROM pg_logical_slot_get_changes('test_slot_1', NULL, NULL);

TRAP: failed Assert("MyReplicationSlot == NULL"), File: "slot.c",
Line: 638, PID: 80308
postgres: vignesh postgres [local] SELECT(ExceptionalCondition+0xba)
[0x642e7b2ebae1]
postgres: vignesh postgres [local] SELECT(ReplicationSlotAcquire+0x6e)
[0x642e7b00d732]

Regards,
Vignesh

---

From: SATYANARAYANA NARLAPURAM <[email-masked]>
Date: Thu, 21 May 2026 07:38:53 -0700

Hi

On Wed, May 20, 2026 at 11:49 PM vignesh C <[email-masked]> wrote:

> > > 2026-05-09 19:45:06.619 UTC [1096805] STATEMENT:  SELECT
> pg_create_logical_replication_slot('adv_test', 'test_decoding');
> Line: 638, PID: 1096805

Thank you for letting me know. Fixing these cases in the next update, will
send it shortly.

Thanks,
Satya

---

From: shveta malik <[email-masked]>
Date: Fri, 22 May 2026 14:45:50 +0530

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

thanks
Shveta

---

From: SATYANARAYANA NARLAPURAM <[email-masked]>
Date: Mon, 25 May 2026 00:12:34 -0700

Hi

> pg_logical_slot_peek_binary_changes

Please find the attached v2 patch that addressed these three cases as well.

Thanks,
Satya

---

From: shveta malik <[email-masked]>
Date: Mon, 25 May 2026 15:28:26 +0530

> 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.
Shveta

---

From: SATYANARAYANA NARLAPURAM <[email-masked]>
Date: Mon, 25 May 2026 12:01:37 -0700

Hi,

> +    WHERE slot_name = 'regression_slot_t3';

Thanks for reviewing. Please review the attached v3 patch.

Thanks,
Satya

---

From: shveta malik <[email-masked]>
Date: Tue, 26 May 2026 10:06:24 +0530

> 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.

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Tue, 26 May 2026 11:20:03 +0530

> 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

---

From: SATYANARAYANA NARLAPURAM <[email-masked]>
Date: Tue, 26 May 2026 01:11:36 -0700

Hi,

On Mon, May 25, 2026 at 10:50 PM shveta malik <[email-masked]>
wrote:

> git am
> v3-0001-Release-replication-slot-on-error-in-slot-SQL-functions.patch
Thanks , Shveta! Please find the attached v4 patch that addressed your
comments.

Thanks,
Satya

---

From: shveta malik <[email-masked]>
Date: Wed, 27 May 2026 09:23:54 +0530

On Tue, May 26, 2026 at 1:41 PM SATYANARAYANA NARLAPURAM
<[email-masked]> wrote:
>
> Hi,
>
> On Mon, May 25, 2026 at 10:50 PM shveta malik <[email-masked]> wrote:
>>
> 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-masked]>
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

---

From: SATYANARAYANA NARLAPURAM <[email-masked]>
Date: Tue, 26 May 2026 21:31:45 -0700

Hi Shveta,

> slot, is that intentional? I think creating a permanent slot here will

No specific reason to use temp slot, ok to create a permanent slot too.

> what you think is okay. I have no more comments.

Thank you for the changes and review.

Thanks,
Satya

---

From: Fujii Masao <[email-masked]>
Date: Wed, 27 May 2026 17:11:55 +0900

> Thank you for the changes and review.

When I applied the v4 patch together with Shveta's diff patch and
ran the regression tests, the tests failed.

Could pg_create_physical_replication_slot() still have the same issue
if it throws an error after ReplicationSlotCreate() and that error is
caught by a PL/pgSQL EXCEPTION block?

Also, do maybe pg_copy_physical_replication_slot(), pg_drop_replication_slot(),
and ALTER_REPLICATION_SLOT potentially have the same issue as well?

Regards,

-- 
Fujii Masao

---

From: shveta malik <[email-masked]>
Date: Wed, 27 May 2026 16:29:59 +0530

> ran the regression tests, the tests failed.

That is because my top-up patch lacks slot.out changes, I wanted Satya
to first confirm if the changes are acceptable to him. Attached
another top-up patch for test-output correction.

> and ALTER_REPLICATION_SLOT potentially have the same issue as well?

pg_copy_physical_replication_slot() should not have it as the common
'copy_replication_slot' is already fixed in the patch.  I will review
the others.

thanks
Shveta
From fccb0438213a82af582f5d3a11c09a0060999565 Mon Sep 17 00:00:00 2001
From: Shveta Malik <[email-masked]>
Date: Wed, 27 May 2026 16:21:02 +0530
Subject: [PATCH] test result changes

---
 contrib/test_decoding/expected/slot.out | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/contrib/test_decoding/expected/slot.out b/contrib/test_decoding/expected/slot.out
index f4cb234028b..3a04d2fbe67 100644
--- a/contrib/test_decoding/expected/slot.out
+++ b/contrib/test_decoding/expected/slot.out
@@ -481,6 +481,7 @@ EXCEPTION WHEN OTHERS THEN
 END;
 $$;
 NOTICE:  caught: could not access file "nonexistent_plugin_xyz": No such file or directory
+-- 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';
  slot_was_dropped 
@@ -488,7 +489,8 @@ SELECT count(*) = 0 AS slot_was_dropped FROM pg_replication_slots
  t
 (1 row)
 
-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);
  ?column? 
 ----------
  init
@@ -510,7 +512,7 @@ EXCEPTION WHEN OTHERS THEN
 END;
 $$;
 NOTICE:  caught expected error
--- 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());
      slot_name      
 --------------------
-- 
2.34.1

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Wed, 27 May 2026 11:46:51 +0000

On Wednesday, May 27, 2026 7:00 PM shveta malik <[email-masked]> wrote:
> > Also, do maybe pg_copy_physical_replication_slot(),
> > pg_drop_replication_slot(), and ALTER_REPLICATION_SLOT potentially have
> the same issue as well?
> >
> 
> 'copy_replication_slot' is already fixed in the patch.  I will review the others.

I have one slight concern about the approach of releasing the slot within a
PG_CATCH() block in lots of functions. I'm not entirely sure if it's safe or
acceptable to do so before aborting the current transaction, so just to confirm
it once:

Since both ReplicationSlotRelease() and ReplicationSlotDropPtr() acquire
LWLocks, it's possible that a backend reports an ERROR while already holding
one of these locks, then enters the PG_CATCH() block and calls
ReplicationSlotRelease(), which attempts to acquire the same LWLock. However,
LWLocks do not distinguish between locks held by the same backend versus other
backends, so the backend could block forever and become uninterruptible.

I don't have a better alternative, but I think we can
evaluate once whether this is a real risk and if it's acceptable (perhaps the
scenario is rare enough to be acceptable). It may also be worth adding comments
to document this risk.

Best Regards,
Hou zj

---

From: Fujii Masao <[email-masked]>
Date: Wed, 27 May 2026 21:10:01 +0900

On Wed, May 27, 2026 at 8:00 PM shveta malik <[email-masked]> wrote:
> 'copy_replication_slot' is already fixed in the patch.

copy_replication_slot() calls create_physical_replication_slot() before
entering the PG_TRY/PG_CATCH block. So if create_physical_replication_slot()
throws an error, wouldn't the same issue still occur?

>  I will review

Thanks!

Regards,

-- 
Fujii Masao

---

From: SATYANARAYANA NARLAPURAM <[email-masked]>
Date: Wed, 27 May 2026 18:08:21 -0700

Hi,

On Wed, May 27, 2026 at 1:12 AM Fujii Masao <[email-masked]> wrote:

> caught by a PL/pgSQL EXCEPTION block

> pg_drop_replication_slot(),

Addressed these in v5 patch, will send out shortly. ALTER_REPLICATION_SLOT
is not exploitable by a SQL query though it has a similar signature.
A walsender error terminates the session so there is no session to leave in
a bad state.

---

From: SATYANARAYANA NARLAPURAM <[email-masked]>
Date: Wed, 27 May 2026 18:11:16 -0700

Hi

> another top-up patch for test-output correction.

Thanks for the patches, I combined these changes in my latest patch. Please
find the v5.

 Thanks,
Satya

---

From: Fujii Masao <[email-masked]>
Date: Fri, 29 May 2026 13:17:17 +0900

On Thu, May 28, 2026 at 10:11 AM SATYANARAYANA NARLAPURAM
<[email-masked]> wrote:
> Thanks for the patches, I combined these changes in my latest patch. Please find the v5.

Thanks for updating the patch! But, v5 patch caused a compilation failure.

slotfuncs.c:119:32: error: too few arguments to function call, single
argument 'try_disable' was not specified
  119 |                         ReplicationSlotDropAcquired();
      |                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
../../../src/include/replication/slot.h:338:13: note:
'ReplicationSlotDropAcquired' declared here
  338 | extern void ReplicationSlotDropAcquired(bool try_disable);
      |             ^                           ~~~~~~~~~~~~~~~~
slotfuncs.c:207:32: error: too few arguments to function call, single
argument 'try_disable' was not specified
  207 |                         ReplicationSlotDropAcquired();
      |                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
../../../src/include/replication/slot.h:338:13: note:
'ReplicationSlotDropAcquired' declared here
  338 | extern void ReplicationSlotDropAcquired(bool try_disable);
      |             ^                           ~~~~~~~~~~~~~~~~
slotfuncs.c:922:32: error: too few arguments to function call, single
argument 'try_disable' was not specified
  922 |                         ReplicationSlotDropAcquired();
      |                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
../../../src/include/replication/slot.h:338:13: note:
'ReplicationSlotDropAcquired' declared here
  338 | extern void ReplicationSlotDropAcquired(bool try_disable);
      |             ^                           ~~~~~~~~~~~~~~~~
3 errors generated.

Regards,

-- 
Fujii Masao

---

From: SATYANARAYANA NARLAPURAM <[email-masked]>
Date: Thu, 28 May 2026 22:11:14 -0700

Hi

>   119 |                         ReplicationSlotDropAcquired();

Please see the v6 patch. Upstream commit 2af1dc89282 changed the
ReplicationSlotDropAcquired signature since the patch generated.

Thanks,
Satya

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Fri, 29 May 2026 05:36:49 +0000

> Thanks for the patches, I combined these changes in my latest patch. Please find the v5.

Thanks for updating the patch.

Few comments:

1.

The patch places PG_CATCH() inside create_logical_replication_slot(), but the
caller may still error out before releasing the slot. It would be better to
catch the error at the caller level instead. The same problem exists in
copy_replication_slot(), which does not include the slot persistence logic
within its CATCH block.

2.

I have a concern about the LWLock handling added within the PG_CATCH block, see [1].

[1] https://www.postgresql.org/message-id/TY4PR01MB17718F04D32E0073F0BC7EC0294082%40TY4PR01MB17718.jpnprd01.prod.outlook.com

Best Regards,
Hou zj

---

From: shveta malik <[email-masked]>
Date: Fri, 29 May 2026 11:15:25 +0530

> throws an error, wouldn't the same issue still occur?

You are right. Using v5, if I force create_physical_replication_slot()
to fail while executing pg_copy_physical_replication_slot() (through
debugging), I can see that the next slot-related call hits an Assert.

1)
I also noticed that for pg_copy_logical_replication_slot(), we do not
hit the CATCH block of copy_replication_slot(), but instead the one in
create_logical_replication_slot(). That behavior seems fine.

However, I noticed some inconsistencies in the implementation.

For create_physical_replication_slot(), the caller
pg_create_physical_replication_slot() contains the TRY-CATCH block,
whereas create_logical_replication_slot() contains its own TRY-CATCH
block internally.

I think it makes more sense to keep the TRY-CATCH handling inside the
internal functions, i.e. create_logical_replication_slot() and
create_physical_replication_slot(), since that would automatically
cover all callers. For example, create_logical_replication_slot() is
invoked from multiple places, so callers need not worry about cleanup
handling themselves. Similarly, for
create_physical_replication_slot(), we could move the TRY-CATCH block
inside the function instead of having it in
pg_create_physical_replication_slot(). Doing so would also resolve the
issue with pg_copy_physical_replication_slot().

2)
I also feel that ReplicationSlotCreate() should be moved inside the
TRY block in create_logical_replication_slot().

Currently, if in the future ReplicationSlotCreate() gains any
post-slot-creation implementation that could throw an error, we may
end up leaving the system in an unsafe state. Keeping it inside the
TRY block would make the code more robust against such future changes.
~~

Reveiwign further. Need to review few more things on v5/v6.

thanks
Shveta

---

From: SATYANARAYANA NARLAPURAM <[email-masked]>
Date: Tue, 9 Jun 2026 21:06:29 -0700

Hi

> Reveiwign further. Need to review few more things on v5/v6.

Got stuck with something. Will send a revised patch over the weekend, in
the meantime if you want to take it forward please feel free to.

Thanks,
Satya

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Wed, 10 Jun 2026 15:14:00 +0900 (JST)

Sorry for the late response.

I may be misunderstanding the patch, but it looks to me as if it is
trying to handle several different classes of problems uniformly by
wrapping them in exception handling.

ReplicationSlotAcquire() can exit with an ERROR after setting
MyReplicationSlot. I think it should restore that state itself.  Or
rather, perhaps it should not set MyReplicationSlot until it is
certain that the slot can be successfully acquired.  I think the same
kind of argument applies to ReplicationSlotCreate().

There is also a different issue around pgstat_create_replslot().  If
ReplicationSlotCreate() calls it and then an error is thrown later in
the caller, the caller needs to clean up that pgstat entry.  That
seems like a separate kind of cleanup from restoring
MyReplicationSlot.

In pg_logical_slot_get_changes_guts(), the patch moves the call to
ReplicationSlotAcquire() inside the PG_TRY block.  In practice the
code checks whether MyReplicationSlot is NULL before releasing it, so
this may work.  However, semantically, it looks as if the caller is
also responsible for releasing the slot even when
ReplicationSlotAcquire() itself fails.

More generally, I am not sure that all of these cleanup actions belong
at the same level.

Restoring MyReplicationSlot, cleaning up pgstat entry, and releasing
an acquired slot seem to be different kinds of
responsibilities. Handling them all through the same PG_TRY/PG_CATCH
blocks causes those cleanup responsibilities to cross component
boundaries. That, in turn, makes it harder to tell whether all
resources affected by an error are actually being cleaned up.

Regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center