Hi,
On Thu, Sep 3, 2026 at 1:58 PM Masahiko Sawada <[email protected]> wrote:
>
> Thank you for updating the patch! Here are some review comments:
>
> + if (isCommit)
> + {
> + Assert(MyReplicationSlot != NULL);
> + ereport(WARNING,
> + (errcode(ERRCODE_WARNING),
> + errmsg("subtransaction left replication slot \"%s\"
> acquired",
> + NameStr(MyReplicationSlot->data.name)),
> + errhint("Check for missing \"ReplicationSlotRelease\"
> calls.")));
>
> The hint message "Check for missing ReplicationSlotRelease call" seems
> to be for us (PostgreSQL hackers) but not users. I think such messages
> should be left as a comment instead of in errhint.
Agreed. Will fix it.
> Also, we don't prohibit external extensions or functions to commit a
> subtransaction while holding a replication slot. If there are such
> extensions, users would get WARNING messages. Which seems to be
> something I'd like to avoid in minor releases.
Thanks Sawada-san for the off-list discussion on this point. Here is a
summary for others' input.
There can be two cases for external modules implementing logical
decoding functionality. A function that unknowingly forgets to call
ReplicationSlotRelease(), and a function that intentionally holds the
slot across subxact boundaries and releases it later in the top-level
transaction. For example
```
BeginInternalSubTransaction("xxx");
ReplicationSlotAcquire(name, ...);
<do something>
ReleaseCurrentSubTransaction();
<do more something>
ReplicationSlotRelease();
```
The above seems like a legitimate usage (though we don't know if there
is any real user of this pattern today). We can't easily distinguish
between the two cases in the subxact commit path. The first case is
more of a coding and reviewing problem. In both cases, calling the
function twice in a row would hit Assert(MyReplicationSlot == NULL) or
silently overwrite the slot, but the intentional case must already be
aware of this. Even if the core emits a WARNING and users report it,
there may not be anything we can do about it. If they release the slot
at the end of the function, it is not a problem. If they forget, they
need to fix it themselves.
Given all this, emitting a WARNING on a subxact commit may not seem
right even on HEAD. Silently handing off the slot to the parent
transaction on subxact commit seems like the better approach.
Would like to hear if others think differently.
> ---
> +-- Error raised inside a PL/pgSQL block with an EXCEPTION clause is caught
> in a
> +-- subtransaction; the slot must still be released.
> +SELECT 'init' FROM
> pg_create_logical_replication_slot('regress_subxact_slot',
> 'test_decoding');
> +DO $$
> +BEGIN
> + PERFORM pg_replication_slot_advance('regress_subxact_slot', '0/1');
> +EXCEPTION WHEN OTHERS THEN
> + RAISE NOTICE 'caught SQLSTATE %', SQLSTATE;
> +END $$;
> +SELECT count(*) >= 0 AS peek_ok
> + FROM pg_logical_slot_peek_changes('regress_subxact_slot', NULL, NULL);
>
> The last sentence is the comment doesn't match the test well and this
> test doesn't fail on non-assertion builds. I think we can check the
> active column in pg_replication_slots instead or before
> slot_peek_changes() call.
Nice catch! Will fix it.
--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com
