On Wed, Aug 12, 2026 at 8:31 PM shveta malik <[email protected]> wrote: > > On Thu, Aug 13, 2026 at 7:01 AM Bharath Rupireddy > <[email protected]> wrote: > > > > Hi, > > > > On Mon, Aug 10, 2026 at 11:16 PM shveta malik <[email protected]> > > wrote: > > > > > > > I will drop both asserts and > > > > keep a short comment explaining why the slot is still held here. The > > > > existing AtEOSubXact_LargeObject() and AtEOSubXact_Files() don't check > > > > the passed-in mySubid for invalid either. > > > > > > > > Does the following work for you? > > > > > > > > /* > > > > * The aborting subxact is the one that acquired the slot, so the slot is > > > > * still held and must be released. acquiredInSubId is set only when a > > > > slot > > > > * is held and cleared when it is released, so a matching subxact id > > > > means > > > > * the slot is ours. > > > > */ > > > > ReplicationSlotRelease(); > > > > > > I am okay with this comment. No 'MyReplicationSlot-null' check and no > > > assert. > > > > Thanks. Done so in the attached v12 patch. Please have a look. > > > > Thanks. Looks good. I have no further comments.
Thank you for updating the patch! I reviewed the v12 patch and here are some review comments: +static SubTransactionId acquiredInSubId = InvalidSubTransactionId; I'm not sure this variable name is ideal, since "acquired..." can be read as a boolean. How about something like MyReplicationSlotSubid or slotAcquireSubid? --- +/* + * Release the replication slot at subxact end if it was acquired here. + * + * A slot function acquires a slot and releases it before returning. On error + * the top-level error handler releases it. But PL/pgSQL, PL/Perl, PL/Python and + * PL/Tcl run an error-handling block in an internal subxact, and when an error + * there is caught the top-level handler is never reached, so the slot would + * otherwise stay acquired. Release it when the subxact that acquired it aborts, + * the same way AtEOSubXact_LargeObject() and other subxact-scoped resources are + * handled. The subxact id is used rather than a nesting level because levels + * are reused across subxacts while ids are not. + */ I don't think it's the right place to explain the bug in detail, and mentioning AtEOSubXact_LargeObject() seems unnecessary. How about rewriting it to something like: /* * At subxact end, hand off or release MyReplicationSlot if it was acquired * in this subxact. On commit, ownership passes to the parent subxact; on * abort, the slot is released (a dnthe sessions' temp slots dropped). */ --- + /* + * The aborting subxact is the one that acquired the slot, so the slot is + * still held and must be released. acquiredInSubId is set only when a + * slot is held and cleared when it is released, so a matching subxact id + * means the slot is ours. + */ + ReplicationSlotRelease(); We should add an assertion that MyReplicationSlot is not NULL before this call. --- AtEOSubXact_ReplicationSlot() performs the same slot cleanup (release + drop temporary slots) that the error path in PostgresMain() does. It would be good to add a note around the ReplicationSlotRelease()/ReplicationSlotCleanup() calls in postgres.c so that any future change there is also considered for AtEOSubXact_ReplicationSlot() (and vice versa). Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
