pg_createsubscriber does not check output_plugin_libraries

On Fri, Sep 4, 2026 at 12:49 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

Possible fix
=======
I think pg_createsubscriber can check the parameter in check_publisher().
Attached 0002 patch does the same.

Thanks for the patch!

I have a few comments.

Regarding the pg_createsubscriber docs, isn't it better to mention
output_plugin_libraries in the Prerequisites section?

* - max_replication_slots >= current + number of dbs to be converted
* - max_wal_senders >= current + number of dbs to be converted
* - max_slot_wal_keep_size = -1 (to prevent deletion of required WAL files)
* -----------------------------------------------------------------------

check_publisher() has the above source comment. Shouldn't
it mention output_plugin_libraries as well?

+ if (!SplitGUCList(output_plugin_libraries, ',', &allowed_plugins))
+ {
+ /*
+ * Should not happen. (Frontend and backend GUC_LIST_QUOTE parsing
+ * have to remain compatible for pg_dump at minimum.)
+ */
+ pg_fatal("could not parse \"output_plugin_libraries\" setting '%s'",
+ output_plugin_libraries);

Since SplitGUCList() may modify its input, output_plugin_libraries
might no longer show the orignal GUC value when it's passed to
pg_fatal(). Isn't it better to follow pg_upgrade/check.c and pass
a separate copy to SplitGUCList(), keeping output_plugin_libraries
unchanged for error reporting?

-  # Note that src/bin/pg_upgrade/check.c assumes GUC_LIST_QUOTE here.
+  # Note that src/bin/pg_upgrade/check.c and
src/bin/pg_basebackup/pg_createsubscriber assume GUC_LIST_QUOTE here.

"pg_createsubscriber" should be "pg_createsubscriber.c" here?

Note
====
BTW, I noticed that `char *max_slot_wal_keep_size` is pg_strdup'd but not free'd.
0003 fixes that.

LGTM. We can commit this together with the 0002 patch.

Regards,

--
Fujii Masao

Dear Fujii-san,

Thanks for reviewing. I think your points are correct. PSA new version.
Only needed patches were attached.

IIUC the same issue could happen till PG18. Not sure the case when
GUC was added to the old version, but I feel it could be backpatched.
Patches for PG18 were also attached. For PG19, same ones as HEAD
were usable.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Fri, Sep 4, 2026 at 8:47 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

Dear Fujii-san,

Thanks for reviewing. I think your points are correct. PSA new version.
Only needed patches were attached.

Thanks for updating the patches!

I found that output_plugin_libraries is PGC_SUSET, so its value may
differ between databases. But, with the patch, pg_createsubscriber
seems to check output_plugin_libraries only in the database specified by
conninfo, not in the databases specified by --database, where the
subscriptions are actually created.

If output_plugin_libraries does not include pgoutput in one of the
databases specified by --database, --dry-run could succeed, while the
actual run could fail because pgoutput is not allowed in that database.
Isn't this a problem?

-  # Note that src/bin/pg_upgrade/check.c assumes GUC_LIST_QUOTE here.
+  # Note that src/bin/pg_upgrade/check.c and
src/bin/pg_basebackup/pg_createsubscriber.c assume GUC_LIST_QUOTE
here.

For the v18 patch, should the corresponding comment also be updated in
src/backend/utils/misc/guc_tables.c?

/* note: src/bin/pg_upgrade/check.c assumes GUC_LIST_QUOTE here */
GUC_LIST_INPUT | GUC_LIST_QUOTE | GUC_SUPERUSER_ONLY

IIUC the same issue could happen till PG18. Not sure the case when
GUC was added to the old version, but I feel it could be backpatched.
Patches for PG18 were also attached. For PG19, same ones as HEAD
were usable.

What about v17? We should backpatch this to v17 as well, since both
output_plugin_libraries and pg_createsubscriber are supported there?

Regards,

--
Fujii Masao

Dear Fujii-san,

I found that output_plugin_libraries is PGC_SUSET, so its value may
differ between databases. But, with the patch, pg_createsubscriber
seems to check output_plugin_libraries only in the database specified by
conninfo, not in the databases specified by --database, where the
subscriptions are actually created.

If output_plugin_libraries does not include pgoutput in one of the
databases specified by --database, --dry-run could succeed, while the
actual run could fail because pgoutput is not allowed in that database.
Isn't this a problem?

Right. After considering more, ALTER DATABASE SET command allows to set
different parameters for new connections.
I added check_publisher_per_database() to iterate all databases and check the parameter.
Alternative way I considered was to put in setup_publisher() because it also
connects to all given databases, but I preferred to handle at the verification
phase.

-  # Note that src/bin/pg_upgrade/check.c assumes GUC_LIST_QUOTE here.
+  # Note that src/bin/pg_upgrade/check.c and
src/bin/pg_basebackup/pg_createsubscriber.c assume GUC_LIST_QUOTE
here.

For the v18 patch, should the corresponding comment also be updated in
src/backend/utils/misc/guc_tables.c?

Fixed.

What about v17? We should backpatch this to v17 as well, since both
output_plugin_libraries and pg_createsubscriber are supported there?

Right. I misunderstood that the command was introduced in PG18.
Attached accordingly.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Dear hackers,

I found the content was truncated. There might be an environment issue on my side.
PSA correct ones.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Mon, Sep 7, 2026 at 3:05 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

Dear hackers,

I found the content was truncated. There might be an environment issue on my side.
PSA correct ones.

A small comment.
in the patch on HEAD, the verb "assume" was correctly used here:
Note that src/bin/pg_upgrade/check.c and
src/bin/pg_basebackup/pg_createsubscriber.c assume GUC_LIST_QUOTE
here.

But in the patch for PG17 and PG18, the verb assumes was incorrectly
used. It should be "assume"
PG17: /* note: src/bin/pg_upgrade/check.c and
src/bin/pg_basebackup/pg_createsubscriber.c assumes GUC_LIST_QUOTE
here */
PG18: /* note: src/bin/pg_upgrade/check.c and
src/bin/pg_basebackup/pg_createsubscriber.c assumes GUC_LIST_QUOTE
here */

regards,
Ajin Cherian
Fujitsu Australia

On Tue, Sep 8, 2026 at 1:46 PM Ajin Cherian <itsajin@gmail.com> wrote:

On Mon, Sep 7, 2026 at 3:05 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

Dear hackers,

I found the content was truncated. There might be an environment issue on my side.
PSA correct ones.

Thanks for updating the patches!

I did some further review.

The patched pg_createsubscriber seems to assume that the server
supports output_plugin_libraries. However, a user may run a newer
version of pg_createsubscriber against a server running an older minor
version that does not yet support output_plugin_libraries (e.g., v18.4).
So, for the v17 and v18 versions of pg_createsubscriber, it should handle
this case?

Previously, a non-superuser without permission to access
output_plugin_libraries could run pg_createsubscriber successfully.
But, with the patch, it fails with a permission denied error. I think
we should avoid this, for example by skipping the check when the user
doesn't have sufficient permission, rather than adding a new prerequisite
for running pg_createsubscriber. Thoughts?

+ /* Also check per-database settings on the publisher */
+ check_publisher_per_database(dbinfo);

In the v19 and v20 patches, this check is called from
check_publisher(), whereas in the v17 and v18 patches it is called from
check_subscriber(). Could you tell me why they are different?

Regards,

--
Fujii Masao

Dear Fujii-san,

The patched pg_createsubscriber seems to assume that the server
supports output_plugin_libraries. However, a user may run a newer
version of pg_createsubscriber against a server running an older minor
version that does not yet support output_plugin_libraries (e.g., v18.4).
So, for the v17 and v18 versions of pg_createsubscriber, it should handle
this case?

Previously, a non-superuser without permission to access
output_plugin_libraries could run pg_createsubscriber successfully.
But, with the patch, it fails with a permission denied error. I think
we should avoid this, for example by skipping the check when the user
doesn't have sufficient permission, rather than adding a new prerequisite
for running pg_createsubscriber. Thoughts?

Good point, should be fixed. I came up with an idea to spedcify missing_ok := true
for current_setting(), and skip checking if it returns NULL. Thought?

+ /* Also check per-database settings on the publisher */
+ check_publisher_per_database(dbinfo);

In the v19 and v20 patches, this check is called from
check_publisher(), whereas in the v17 and v18 patches it is called from
check_subscriber(). Could you tell me why they are different?

It was not intended.
The git am command on my env put the code at the wrong place, so it was the reason.
The function referred dbinfo[i].pubconninfo, so the behavior is the same. Moved
to the correct place.

Attached new patch set. I also noticed that max_slot_wal_keep_size is not checked
in PG17, so 0002 was removed.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Tue, Sep 8, 2026 at 7:52 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

Good point, should be fixed. I came up with an idea to spedcify missing_ok := true
for current_setting(), and skip checking if it returns NULL. Thought?

Using missing_ok = true seems fine for older minor versions that don't
support output_plugin_libraries, but it doesn't handle insufficient
privileges cases, does it? current_setting('output_plugin_libraries', true)
still raises a permission-denied error if the current user cannot examine
the setting, no?

How about querying pg_settings instead? For example,

SELECT setting
FROM pg_catalog.pg_settings
WHERE name = 'output_plugin_libraries';

This returns no rows if the parameter doesn't exist or the current user
doesn't have permission to examine it, so we could skip the check for
that database in either case.

Regards,

--
Fujii Masao

On Thu, Sep 3, 2026 at 8:49 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

Hi hackers,
(CC: Jacob, who was an author of 226e49cb)

Hi! Thanks for the report, and sorry for the delay over the US holiday weekend.

226e49cb added a GUC parameter to specify trusted output plugins, and slot creation
would fail if a give plugin is not listed there.

Right. (pgoutput is in that parameter by default, so the case in
question requires a DBA to remove it and then be surprised at the
results.)

pg_createsubscriber command creates replication slots with plugin 'pgoutput',
without checking the GUC. This meant if the plugin name is not specified in the
parameter, --dry-run mode passes but actual convertion fails.
It's very surprising for users and should be avoided.

Isn't this the same behavior as for all other permissions issues? We
don't check for
- REPLICATION privs
- permission for FOR ALL TABLES
- CREATE on the target databases
- ownership on a publication to drop
- permission to call pg_log_standby_snapshot()

Looks like all those pass a --dry-run and then fail later.

No worries if we're trying to improve this behavior incrementally; I'm
just trying to understand what we want the standard to be for
pg_createsubscriber behavior going forward.

Thanks,
--Jacob

Dear Fujii-san, Jacob,

Thanks for giving feedbacks. I'm confusing and let me confirm few points.
The situation might be different before and after the PG19.

For PG17 and 18, FOR ALL TABLES publications for publisher databases,
it meant superuser is required for the connection. IIUC superuser can bypass all
permission checks thus it might be OK not to check these points.
(We may have to check it though).
As for the PG19 and HEAD, we allowed to re-use existing publications [1]https://github.com/postgres/postgres/commit/85ddcc2f4cdef490276d151c80459e287bceb782, it
effectively allowed to use non-superuser for connecting to the publisher.
However it meant additional permission checks might be needed like Jacob pointed
out.

In this thread I do not want to broaden for other points and focus on the added
GUC, thought? In other threads we can improve --check option more.

Based on that, permission check is actually needed for PG19/HEAD same check
Fujii-san pointed out. For PG18/17, we may not have to take care the usage of
current_setting() but same code is used for better understanding. Attached patch
set does accordingly. v5-0001 also has a test for the case just in case, but not
sure it should be pushed.

[1]: https://github.com/postgres/postgres/commit/85ddcc2f4cdef490276d151c80459e287bceb782

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Wed, Sep 9, 2026 at 3:33 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

Thanks for giving feedbacks. I'm confusing and let me confirm few points.
The situation might be different before and after the PG19.

Oh, that's a good point.

In this thread I do not want to broaden for other points and focus on the added
GUC, thought? In other threads we can improve --check option more.

As long as everyone is in agreement that --dry-run *should* be
performing those checks in the first place, I'm happy. I just wasn't
sure from looking at the code if it was intended to perform any
permissions checks at all, so I was trying to figure out what my
original patch should have done differently.

Even for PG17, specifying a non-superuser in the connection string
leads to a successful --dry-run, then fails during normal operation.
If our answer to that is "don't do that in the back branches", fine by
me -- but then we could also say "don't remove pgoutput from
output_plugin_libraries and then be surprised when all subscription
operations fail." (The role of each builtin plugin should probably be
better explained in the documentation for output_plugin_libraries,
since this is a brand-new consideration for most people...)

Thanks,
--Jacob

On Wed, Sep 9, 2026 at 5:43 AM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:

(The role of each builtin plugin should probably be
better explained in the documentation for output_plugin_libraries,
since this is a brand-new consideration for most people...)

Splitting this concern off from the original report: I'd like to
backpatch something like the attached so that it's more obvious why
you might (or might not) want to keep those in the list in the first
place.

WDYT?

Thanks,
--Jacob

Hi,

On Thu, Sep 10, 2026 at 9:47 AM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:

On Wed, Sep 9, 2026 at 5:43 AM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:

(The role of each builtin plugin should probably be
better explained in the documentation for output_plugin_libraries,
since this is a brand-new consideration for most people...)

Splitting this concern off from the original report: I'd like to
backpatch something like the attached so that it's more obvious why
you might (or might not) want to keep those in the list in the first
place.

WDYT?

+1 to enhance the docs here. Overall it looks good to me. One nit:

1/ "publication slot creation" seems a bit confusing. How about we say
"Removing pgoutput from output_plugin_libraries will cause future
subscription connections and replication slot creations on the
publisher to fail."

+             Core plugin for logical replication. Removing
<literal>pgoutput</literal>
+             from <literal>output_plugin_libraries</literal> will cause future
+             subscription connections, and publication slot creation, to fail
+             with an error. (Beware that existing replication tools may not
+             degrade gracefully if the server is configured in this way.)

Just curious, do we need to backpatch this to all the supported
branches? Removing these can have consequences that show up later, so
backpatching makes sense to me.

--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com

As long as everyone is in agreement that --dry-run *should* be
performing those checks in the first place, I'm happy.

Yes, it's my intention. Fujii-san, how do you feel? If you're OK, we can proceed.

Even for PG17, specifying a non-superuser in the connection string
leads to a successful --dry-run, then fails during normal operation.

To confirm, is it allowed to add such a verification in back branches?
I imagined running something like below after connecting to the publisher.

```
SELECT rolsuper
FROM pg_catalog.pg_roles
WHERE rolname = session_user;
```

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Fri, Sep 11, 2026 at 11:51 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

As long as everyone is in agreement that --dry-run *should* be
performing those checks in the first place, I'm happy.

Yes, it's my intention. Fujii-san, how do you feel? If you're OK, we can proceed.

I'm not sure if we have a clear policy on what --dry-run should check.
Personally, I'd like it to check as many prerequisites as practical.
Ideally, I just wish that a successful dry run would mean that the actual
run will succeed, but that's probably not always possible.

Some checks may be difficult to perform or have undesirable side effects,
especially if they require creating, dropping, or modifying objects. They
may also require complicated code or introduce performance overhead. So I
think it's reasonable to leave such checks out.

Therefore, I'd just favor adding checks that are straightforward and
practical to perform.

TBH, the code for checking output_plugin_libraries seems to be getting
more complicated than I initially expected. But, from my view, it hasn't
become complicated and difficult enough to justify leaving the check out,
at least *for now*.

Even for PG17, specifying a non-superuser in the connection string
leads to a successful --dry-run, then fails during normal operation.

To confirm, is it allowed to add such a verification in back branches?
I imagined running something like below after connecting to the publisher.

Adding such verification would be useful, but it looks like an improvement
rather than a bug fix to me. So, I don't think we can add it to the stable
branches. Thoughts?

Regards,

--
Fujii Masao

Dear Fujii-san,

I'm not sure if we have a clear policy on what --dry-run should check.
Personally, I'd like it to check as many prerequisites as practical.

Yes, same here.

TBH, the code for checking output_plugin_libraries seems to be getting
more complicated than I initially expected. But, from my view, it hasn't
become complicated and difficult enough to justify leaving the check out,
at least *for now*.

OK, so let's keep working on the patch set. Patches in [1]/messages/by-id/OS9PR01MB12149E6DB23FD6B53CCC64403F5B02@OS9PR01MB12149.jpnprd01.prod.outlook.com are still valid.
We can consider again after the patch will be a committable shape.

To confirm, is it allowed to add such a verification in back branches?
I imagined running something like below after connecting to the publisher.

Adding such verification would be useful, but it looks like an improvement
rather than a bug fix to me. So, I don't think we can add it to the stable
branches. Thoughts?

OK, so let's drop the idea for now.

[1]: /messages/by-id/OS9PR01MB12149E6DB23FD6B53CCC64403F5B02@OS9PR01MB12149.jpnprd01.prod.outlook.com

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Dear Jacob,

Splitting this concern off from the original report:

Actually this thread was not split on the web archive ;-(. But the proposal is good.

I'd like to
backpatch something like the attached so that it's more obvious why
you might (or might not) want to keep those in the list in the first
place.

WDYT?

Few comments:

1.
Wondering we need to add descriptions in logical-decoding.sgml. One idea is
something like below, are there better ideas?

```
--- a/doc/src/sgml/logicaldecoding.sgml
+++ b/doc/src/sgml/logicaldecoding.sgml
@@ -783,6 +783,8 @@ DETAIL:  Synchronization could lead to data loss, because the remote slot needs
      <filename>contrib/test_decoding</filename>
     </link>
     subdirectory of the PostgreSQL source tree.
+    When you use new output plugins, you must also update
+    <xref linkend="guc-output-plugin-libraries"/>.
    </para>
```

2.
```
+            <entry>
+             Decodes the WAL stream into a human-readable text representation.
+             <xref linkend="app-pgrecvlogical"/> uses this plugin by default, so
+             removing it from <literal>output_plugin_libraries</literal> will
+             prevent the use of that utility unless a replacement plugin is
+             installed and explicitly selected with
+             <link linkend="app-pgrecvlogical-option-plugin"><literal>--plugin</literal></link>.
+            </entry>
```

The description may not be accurate because the --plugin is referred only at the
creation phase.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Fri, Sep 11, 2026 at 2:45 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

TBH, the code for checking output_plugin_libraries seems to be getting
more complicated than I initially expected. But, from my view, it hasn't
become complicated and difficult enough to justify leaving the check out,
at least *for now*.

OK, so let's keep working on the patch set. Patches in [1] are still valid.
We can consider again after the patch will be a committable shape.

Sounds good.

+ /*
+ * Should not happen. (Frontend and backend GUC_LIST_QUOTE parsing
+ * have to remain compatible for pg_dump at minimum.)
+ */

I spun for a while on the network compatibility implications of this,
until I realized that pg_createsubscriber isn't going to work if the
source server is a different major version (even though it doesn't
explicitly check the source server's version, AFAICT?). If that's
correct, this code should be fine. I haven't reviewed line-by-line,
but the general approach LGTM.

Thanks!
--Jacob