Re: BUG #19500: pgrepack logical decoding plugin can crash assert builds via SQL decoding API

Hi Álvaro,

On Mon, Jun 1, 2026 at 10:43 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:

> On 2026-May-29, Álvaro Herrera wrote:
>
> > On 2026-05-28, PG Bug reporting form wrote:
> >
> > > It appears that the pgrepack output plugin is accessible through the
> > > SQL logical decoding API, even though the plugin code explicitly
> > > indicates that this interface is not supported. Reading changes from
> > > such a slot can cause a backend process crash in builds with asserts
> > > enabled.
> >
> > Yeah, I would like to have a way to prevent this, if only for
> > user-friendliness, but it's not terribly pressing since only a role
> > with REPLICATION privs can create the replication slot, which as I
> > recall are already pretty powerful.
>
> How about something like this?  It makes your test case throw an error
> instead of failing the assertion, which I suppose is an improvement.
>
> The patch is a bit noisy because I moved more code than the minimum
> necessary; but the gist of it is that we allocate RepackDecodingState in
> repack_startup(), then have repack_setup_logical_decoding() fill in a
> magic number, which we later check in repack_begin_txn().  This is a bit
> wasteful, because we have to do that check once for each and every
> transaction; however I see no other callback that would let us do this
> kind of check after the slot is created but before we start to consume
> from it.
>

The magic guard is correct. One thing worth noting: the check is in the
begin callback, which fires only at the transaction's commit, so a single
large transaction (a bulk load) is decoded in full and buffered, spilling to
disk past logical_decoding_work_mem, before the plugin rejects it.
That work is then thrown away. It's a misuse path, so this might not be
a big concern, I guess, but it does mean the wasted work scales with
the transaction size rather than being negligible.

Could we reject the pgrepack plugin at slot creation instead, in
pg_create_logical_replication_slot() and the CREATE_REPLICATION_SLOT
command, so misuse gets a clear "reserved for REPACK (CONCURRENTLY)"
error up front, before any decoding? REPACK creates its slot directly via
ReplicationSlotCreate(), so it's unaffected, and the begin-callback check
with magic guard can stay as the internal safety net.
Happy to be told this isn't worth special-casing :)

I attached the patch which brings the above behaviour, this patch in on top
of your patch.

Thoughts?


-- 
Thanks,
Srinath Reddy Sadipiralla
EDB: https://www.enterprisedb.com/