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