Обсуждение: Document default values for pgoutput options + fix missing initialization for "origin"

Hi,

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

I'd like to propose adding the default values to the documentation to
improve clarity and usability. Patch attached (0001 patch).

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).

Thoughts?

Regards,

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

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

I'd like to propose adding the default values to the documentation to

improve clarity and usability. Patch attached (0001 patch).

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).

On Tue, May 20, 2025 at 8:11 AM Euler Taveira <euler@eulerto.com> 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
>
> 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).
>
> 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.
>

One point to note about this is that we change the default value for
the streaming option to parallel for a subscription in the commit
1bf1140be8. But pgoutput still considers the default value to be off.
I thought about this, but not sure if there is any clear value in
changing the default of pgoutput. Would you have any thoughts on the
same?

--
With Regards,
Amit Kapila.

On 2025/05/20 13:09, Amit Kapila wrote:
>> +       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.
>>
> 
> One point to note about this is that we change the default value for
> the streaming option to parallel for a subscription in the commit
> 1bf1140be8. But pgoutput still considers the default value to be off.
> I thought about this, but not sure if there is any clear value in
> changing the default of pgoutput. Would you have any thoughts on the
> same?

Using "off" as the pgoutput's default seems better to me,
since it works regardless of protocol version.

Regards,

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

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