public inbox for [email protected]
help / color / mirror / Atom feedFrom: Srinath Reddy Sadipiralla <[email protected]>
To: Álvaro Herrera <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: Antonin Houska <[email protected]>
Cc: [email protected]
Subject: Re: BUG #19500: pgrepack logical decoding plugin can crash assert builds via SQL decoding API
Date: Wed, 3 Jun 2026 11:21:17 +0530
Message-ID: <CAFC+b6rpgFUUiU5z_YbF3GJStNqO2Pf7eW96txw=JtEe=_WPzw@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
Hi Álvaro,
On Mon, Jun 1, 2026 at 10:43 PM Álvaro Herrera <[email protected]> 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/
Attachments:
[application/octet-stream] v1-0002-Reject-the-pgrepack-output-plugin-outside-REPACK-CON.patch (4.4K, 3-v1-0002-Reject-the-pgrepack-output-plugin-outside-REPACK-CON.patch)
download | inline diff:
From 456431823892fe98df3fcfa30d8b8c08006d88e6 Mon Sep 17 00:00:00 2001
From: Srinath Reddy Sadipiralla <[email protected]>
Date: Wed, 3 Jun 2026 10:29:30 +0530
Subject: [PATCH] Reject the "pgrepack" output plugin outside REPACK
(CONCURRENTLY)
The "pgrepack" output plugin is an internal component of REPACK
(CONCURRENTLY) and is not meant to be driven through the generic logical
decoding interface.
---
contrib/test_decoding/expected/repack.out | 5 +++++
contrib/test_decoding/sql/repack.sql | 4 ++++
src/backend/replication/slotfuncs.c | 14 ++++++++++++++
src/backend/replication/walsender.c | 10 ++++++++++
4 files changed, 33 insertions(+)
diff --git a/contrib/test_decoding/expected/repack.out b/contrib/test_decoding/expected/repack.out
index 6204e620b43..2732561281b 100644
--- a/contrib/test_decoding/expected/repack.out
+++ b/contrib/test_decoding/expected/repack.out
@@ -99,5 +99,10 @@ REPACK (CONCURRENTLY) repack_conc_replident;
ERROR: cannot execute REPACK (CONCURRENTLY) on relation "repack_conc_replident"
DETAIL: REPACK (CONCURRENTLY) does not support deferrable primary keys.
HINT: Use ALTER TABLE ... REPLICA IDENTITY USING INDEX to designate another index as replica identity.
+-- The "pgrepack" output plugin is internal to REPACK (CONCURRENTLY); it cannot
+-- be selected through the generic logical decoding interface (bug #19500).
+SELECT pg_create_logical_replication_slot('repack_misuse_slot', 'pgrepack');
+ERROR: output plugin "pgrepack" cannot be used through the logical decoding interface
+DETAIL: This output plugin is reserved for internal use by REPACK (CONCURRENTLY).
-- clean up
DROP TABLE repack_conc_replident, clstrpart;
diff --git a/contrib/test_decoding/sql/repack.sql b/contrib/test_decoding/sql/repack.sql
index cea3bd33689..a7cb615e55d 100644
--- a/contrib/test_decoding/sql/repack.sql
+++ b/contrib/test_decoding/sql/repack.sql
@@ -73,5 +73,9 @@ REPACK (CONCURRENTLY) repack_conc_replident;
ALTER TABLE repack_conc_replident ADD PRIMARY KEY (i) DEFERRABLE;
REPACK (CONCURRENTLY) repack_conc_replident;
+-- The "pgrepack" output plugin is internal to REPACK (CONCURRENTLY); it cannot
+-- be selected through the generic logical decoding interface (bug #19500).
+SELECT pg_create_logical_replication_slot('repack_misuse_slot', 'pgrepack');
+
-- clean up
DROP TABLE repack_conc_replident, clstrpart;
diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
index 16fbd383735..17548abf601 100644
--- a/src/backend/replication/slotfuncs.c
+++ b/src/backend/replication/slotfuncs.c
@@ -136,6 +136,20 @@ create_logical_replication_slot(char *name, char *plugin,
Assert(!MyReplicationSlot);
+ /*
+ * The "pgrepack" output plugin is for internal use by REPACK
+ * (CONCURRENTLY) only. Reject it here so that misuse through the SQL
+ * interface fails immediately with a clear message, instead of being
+ * caught later (and only after some decoding work) by the plugin's own
+ * magic-number check in its begin callback. REPACK creates its slot
+ * directly via ReplicationSlotCreate(), so it is unaffected.
+ */
+ if (strcmp(plugin, "pgrepack") == 0)
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("output plugin \"pgrepack\" cannot be used through the logical decoding interface"),
+ errdetail("This output plugin is reserved for internal use by REPACK (CONCURRENTLY)."));
+
/*
* Acquire a logical decoding slot, this will check for conflicting names.
* Initially create persistent slot as ephemeral - that allows us to
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 04aa770d981..b9e52a20173 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -1270,6 +1270,16 @@ CreateReplicationSlot(CreateReplicationSlotCmd *cmd)
CheckLogicalDecodingRequirements(false);
+ /*
+ * The "pgrepack" output plugin is reserved for REPACK (CONCURRENTLY);
+ * reject it here too (see create_logical_replication_slot()).
+ */
+ if (strcmp(cmd->plugin, "pgrepack") == 0)
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("output plugin \"pgrepack\" cannot be used through the logical decoding interface"),
+ errdetail("This output plugin is reserved for internal use by REPACK (CONCURRENTLY)."));
+
/*
* Initially create persistent slot as ephemeral - that allows us to
* nicely handle errors during initialization because it'll get
--
2.43.0
view thread (14+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: BUG #19500: pgrepack logical decoding plugin can crash assert builds via SQL decoding API
In-Reply-To: <CAFC+b6rpgFUUiU5z_YbF3GJStNqO2Pf7eW96txw=JtEe=_WPzw@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox