Re: [PATCH] Explain what the default output_plugin_libraries do

Hi Kuroda-san, Jacob,

Thanks for the patches. I reviewed v7 (CF entry 7271) and tested it
locally. Jacob, I also added my opinion on the new documentation
section you asked about at the end.

What I tested
- v7-0001 and v7-0002 apply cleanly with "git apply" on master at
9f191dce61e (no rebase needed). Build with --enable-cassert
--enable-debug has no warnings.
- v7-0001 does not apply to REL_18_STABLE (f5c41f6681c) or
REL_17_STABLE (702b3dc86d3), which is expected since v6/v7 are
master-only. I did not build the older branches.
- Reproducer (primary plus a physical standby from pg_basebackup, two
databases db1 and db2, and "ALTER DATABASE db2 SET
output_plugin_libraries = 'test_decoding'"):
- Without the patch: "pg_createsubscriber --dry-run --database db1
--database db2" succeeds (rc=0). The real run on a fresh standby then
fails (rc=1) with: could not create replication slot
"pg_createsubscriber_16385_..." in database "db2": ERROR: library
"pgoutput" may not be used as an output plugin.
- With the patch: both the dry run and the real run fail early with
rc=1 and the hint: Add "pgoutput" to the configuration parameter
"output_plugin_libraries".
- After "ALTER DATABASE db2 RESET output_plugin_libraries", the run
works with and without the patch (control).
- make -C src/bin/pg_basebackup check with the patch:
040_pg_createsubscriber.pl passes (63 tests), and the whole directory
passes (6 files, 366 tests).

Comments
1. The check opens one extra connection per database
(check_publisher() already connects to the first one, and
setup_publisher() connects again later). This is not a problem, just
an observation. It also stops at the first database that does not
allow pgoutput, so a user with several such databases sees them one by
one. That seems fine to me.
2. The hint only says to add "pgoutput" to the parameter. Since the
value is often set per database or per role (which is the reason for
the per-database loop), it may help to say that in the message, for
example by mentioning the effective value for that database. Not a
blocker.
3. When the setting cannot be read, the check is skipped silently. I
agree with Kuroda-san that a WARNING without an action for the user is
not great. Maybe a pg_log_debug() line in that branch, so that "-v"
shows that the check was skipped, would be enough.
4. Small cleanup: in the "could not obtain publisher settings" branch,
the result is not cleared before disconnect_database(conn, true). It
exits anyway, so this is harmless.
5. Error messages, the docs addition in pg_createsubscriber.sgml, the
guc_parameters.dat comment and the tests (database-level setting and
the non-superuser case) look good to me. v7-0002 (pg_free of
max_slot_wal_keep_size) is correct and could be committed with 0001.
6. CF process: the entry currently shows "Patch version v2, 1 patch,
+19-0", which is only the documentation patch, so cfbot does not test
the v7 code patch. It may be good to attach v7 (0001 and 0002) to the
thread again together with the doc patch, or split them into two CF
entries.

Backpatch
I agree with Fujii-san that this should be master only. With the
patch, a connection and a query are added to the publisher check, and
the same dry-run limitation already exists for other permission
problems (as Jacob mentioned), so it reads as an improvement to
--dry-run and not as a bug fix.

Jacob's question about the new section "Installing Logical Decoding
Output Plugins" (doc v2)
I only read this patch and did not build the HTML. The content looks
right to me and I think master only is fine for a new section. One
small wording point: "An output plugin must be installed as a
dynamically loadable library" is not true for the built-in pgoutput,
so maybe "A custom output plugin" or "An output plugin that is not
built in".

Overall, the code patch looks good to me and I would be fine with
Ready for Committer after the (optional) comments above.

Best regards,
João Rito

Em ter., 10 de nov. de 2026 às 20:38, Jacob Champion
<jacob(dot)champion(at)enterprisedb(dot)com> escreveu:
>
> On Tue, Sep 22, 2026 at 1:56 PM Jacob Champion
> <jacob(dot)champion(at)enterprisedb(dot)com> wrote:
> > Thanks everybody for the reviews! v2 has the backpatchable piece. It
> > doesn't incorporate Kuroda-san's suggested new section from upthread,
> > since that's master-only and can be committed separately once we have
> > the wording we like.
>
> (This first part has been backpatched; the new draft section remains
> for master. Any other opinions on that one? No rush.)
>
> Thanks,
> --Jacob
>
>
>
>