RE: pg_createsubscriber does not check output_plugin_libraries

Dear Fujii-san,

> + The source server must also allow the <literal>pgoutput</literal> output
> + plugin by setting <xref linkend="guc-output-plugin-libraries"/> to include
> + <literal>pgoutput</literal>.
>
> Isn't it better to clarify that this requirement applies to the effective
> setting for each source database connection? For example:
>
> The effective value of <xref linkend="guc-output-plugin-libraries"/>
> for each connection to a source database must include
> <literal>pgoutput</literal>.

Fixed.

> + if (!SplitGUCList(output_plugin_libraries_copy, ',', &allowed_plugins))
> + {
> + /* Should not happen */
> + pg_fatal("could not parse \"output_plugin_libraries\" setting '%s'
> in database \"%s\"",
> + output_plugin_libraries, dbinfo[i].dbname);
>
> The "Should not happen" comment should be removed because this can happen?
> For example, if output_plugin_libraries is set to 'test_decoding, ' on
> the server, pg_createsubscriber fails to parse it and reaches this point.

Right, removed. I expected that such a setting can be rejected.

> + qr/publisher does not allow the "pgoutput" output plugin in database
> "\Q$db2\E"/,
> + 'primary does not allow to load pgoutput plugin');
>
> "primary" should be "publisher"? For example, how about
> "publisher disallows pgoutput output plugin"?

Some other lines are referred the source as primary, so I think either way is fine.
But fixed accordingly because the line just above uses the term "publisher".

> This comment seems a bit misleading because non-superusers with
> pg_read_all_settings privileges can read this setting. Instead, how about
> "This user cannot read output_plugin_libraries, so the check should be
> skipped even when pgoutput is not allowed."?

Right, fixed. I described correctly in the source but omitted in the test.

> Regarding the backpatch of the 0001 patch, I'm leaning toward treating this
> as an improvement for master only.

It's still debatable point. I felt this parameter was introduced to stable branches
and missed checking in the command.
I'm not sure existing examples we could follow, do you know?

Anyway, I updated a patch set, only for master.

Best regards,
Hayato Kuroda
FUJITSU LIMITED