On Fri, Sep 18, 2026 at 9:49 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> Hi,
>
> On Fri, Sep 18, 2026 at 1:31 AM kedar anavardekar
> <kedar.anavardekar@gmail.com> wrote:
> >
> > Two minor naming suggestions: (please take the suggestions if you
> > think the points are valid)
>
> Thanks for taking a look.
>
> > 1. Could MyReplicationSlotSubId be renamed to
> > MyReplicationSlotSubXactId (or MyReplicationSlotSubTransactionId)
> >
> > SubId may be read as a subscription ID, whereas this variable stores
> > the SubTransactionId of the subtransaction that acquired
> > MyReplicationSlot. The more explicit name would make its purpose
> > clearer and avoid confusion with logical replication subscriptions.
>
> Subscription and its related replication slot on the publisher are on
> two different database instances, and one has the context when reading
> the code around this. Also, "SubId" is used across the code base and I
> want to keep it consistent and short, so MyReplicationSlotSubId looks
> fine to me.
+1
> > 2., could the comment above AtEOSubXact_ReplicationSlot() be revised from:
> > /*
> > * At subxact end, release the replication slot if the subtransaction
> > * where the slot was acquired is aborted.
> > */
> > to:
> > /*
> > * At subxact end, release the replication slot if the subtransaction
> > * in which the slot was acquired is aborted.
> > */
> > “In which” is more precise here because the slot is acquired during
> > that subtransaction.
>
> I believe "where the slot was acquired" is grammatically correct as
> well, so I'm fine with the existing wording.
I reviewed the v17 patch and here are some comments:
for use by the current session. Temporary slots are also
- released upon any error. This function corresponds
+ dropped on any error. An error raised and caught in a
+ subtransaction, for example by a
+ <application>PL/pgSQL</application> exception block, does not
+ drop them. This function corresponds
ISTM what the following sentence says seems to contradict with what
the first sentence says. How about rephrasing it to:
for use by the current session. Temporary slots are also
- released upon any error. This function corresponds
+ dropped when an error is reported to the client. An error
caught inside a
+ subtransaction, for example by a <application>PL/pgSQL</application>
+ exception block, does not drop them. This function corresponds
---
+-- A slot function that errors out must still release the slot, otherwise the
+-- next slot operation in the session fails an assertion or leaks the slot.
+-- Advancing a freshly created slot to a low LSN always errors.
+SELECT 'init' FROM
pg_create_logical_replication_slot('regress_subxact_slot',
'test_decoding');
The comment seems not to be in the right place; it's in right before
the pg_create_logical_replication_slot() call but not related. Given
that we have the comments for subsequent tests, we can remove it.
I've attached the updated patch that incorporated the above comments.
I'm going to push it early next week, barring any objections.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
