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

On 2025/05/21 16:27, Fujii Masao wrote:
> 
> 
> 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.

Pushed. Thanks!


>> 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?

I've applied the patch to master and back-patched it to v16.


>>> 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?

Since this is an improvement rather than a bug fix, I only applied it to master.

Regards,

-- 
Fujii Masao
NTT DATA Japan Corporation