From: Antonin Houska <ah@cybertec.at>
To: Alvaro Herrera <alvherre@kurilemu.de>
cc: Srinath Reddy Sadipiralla <srinath2133@gmail.com>,
    n.kalinin@postgrespro.ru, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #19500: pgrepack logical decoding plugin can crash assert
 builds via SQL decoding API
In-reply-to: <aiCUGyksPoXXolhk@alvherre.pgsql>
References: <aiCUGyksPoXXolhk@alvherre.pgsql>
Comments: In-reply-to Alvaro Herrera <alvherre@kurilemu.de>
   message dated "Wed, 03 Jun 2026 23:02:56 +0200."
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <4413.1780554701.1@localhost>
Content-Transfer-Encoding: quoted-printable
Date: Thu, 04 Jun 2026 08:31:41 +0200
Message-ID: <4414.1780554701@localhost>
Archived-At: 
 <https://www.postgresql.org/message-id/4414.1780554701%40localhost>
Precedence: bulk

Alvaro Herrera <alvherre@kurilemu.de> wrote:

> On 2026-Jun-03, Antonin Houska wrote:
> =

> > Srinath Reddy Sadipiralla <srinath2133@gmail.com> wrote:
> > =

> > > 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 directl=
y 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 :)
> > =

> > Another possible approach: restrict the use of the plugin to the REPAC=
K
> > decoding worker.
> =

> I don't like either of these approaches, because they are forcing the
> generic facility (either slot creation or logical decoding setup) to
> know something about one specific user of the facility.  That is to say,
> the restriction is being added on the wrong side of the abstraction.
> I know my implementation the drawback you (Srinath) mentioned, because
> the abstraction doesn't provide us with a great way to inject an error
> report at the exact spot we need it; but I think it's at the correct
> side of the abstraction.

I noticed that ReplicationSlotAcquire() already does something like that

    /*
     * Do not allow users to acquire the reserved slot. This scenario may
     * occur if the launcher that owns the slot has terminated unexpectedl=
y
     * due to an error, and a backend process attempts to reuse the slot.
     */
    if (!IsLogicalLauncher() && IsSlotForConflictCheck(name))
	ereport(ERROR,
	    errcode(ERRCODE_UNDEFINED_OBJECT),
	    errmsg("cannot acquire replication slot \"%s\"", name),
	    errdetail("The slot is reserved for conflict detection and can only b=
e acquired by logical replication launcher."));


but I agree that it's not perfect to hard-wire particular slot names into
functions like this. Perhaps we could introduce a concept of "reserved slo=
ts"
and an API (callback) to perform these checks, but that's not appropriate =
for
beta release.

> (I'm not really sure that there _is_ a great way to throw an error
> report at the right time.  That would require every single output plugin
> author to add a function we can call; and every single one of them,
> except REPACK, would do nothing.  This seems quite pointless.)
> =

> I frankly don't have a problem with letting a transaction spill a few
> GBs to disk only to then report an error that pgrepack is being misused.
> It's just not something that anyone would do for fun.

I admit that the possibility of wasted processing of a transaction didn't
really frighten me. The idea I posted just occurred to me somehow, but I d=
on't
consider it urgent. I'm fine with your approach.

-- =

Antonin Houska
Web: https://www.cybertec-postgresql.com