Re: Document default values for pgoutput options + fix missing initialization for "origin"

On 2025/05/20 11:40, Euler Taveira wrote:
> On Fri, May 16, 2025, at 12:06 PM, Fujii Masao wrote:
>> The pgoutput plugin options are documented in the logical streaming
>> replication protocol, but their default values are not mentioned.
>> This can be inconvenient for users - for example, when using pg_recvlogical
>> with pgoutput plugin and needing to know the default behavior of each option.
>> https://www.postgresql.org/docs/devel/protocol-logical-replication.html <https://www.postgresql.org/docs/devel/protocol-logical-replication.html>
>>
>> I'd like to propose adding the default values to the documentation to
>> improve clarity and usability. Patch attached (0001 patch).
> 
> Good catch.
> 
> Should we use "on" and "off" as other enum GUCs (wal_compression,
> recovery_prefetch, compute_query_id)? The current convention is to support
> other ways (true / false / 1 / 0) to write boolean but only document one way
> (on / off).

Thanks for the review!

+1. I've updated the patch as you suggested. 0002 patch.


> Since you are changing this page, I would like to suggest removing "Boolean"
> from streaming option. It is not a boolean anymore since protocol version 4.
> The suggested description is:
> 
> +       Option to enable streaming of in-progress transactions. Valid values are
> +       <literal>off</literal> (the default), <literal>on</literal> and
> +       <literal>parallel</literal>. The setting <literal>parallel</literal>
> +       enables sending extra information with some messages to be used for
> +       parallelization. Minimum protocol version 2 is required to turn it
> +       <literal>on</literal>.  Minimum protocol version 4 is required for the
> +       <literal>parallel</literal> value.

Sounds good to me. I created a separate patch for this change
so it can be back-patched independently. 0001 patch. I think
it should be back-patched to v16, where the streaming option
became non-boolean. Thought?


>> While working on this, I also noticed that although most optional parameters
>> (like "binary") are explicitly initialized in parse_output_parameters(),
>> the "origin" parameter is not. Its value (PGOutputData->publish_no_origin)
>> is implicitly set to false due to zero-initialization, but unlike other
>> parameters, it lacks an explicit default assignment.
>>
>> To ensure consistency and make the behavior clearer, I propose explicitly
>> initializing the "origin" parameter as well. A patch for this is also attached
>> (0002 patch).
> 
> LGTM. It seems an oversight from the original commit 366283961ac0.

Yes.
While this issue doesn't cause any practical problems, I think
it's worth back-patching to v16 (where that commit was added)
for clarity. Thoughts?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION