Message-ID: <f6970e1b-e2d9-47bf-9c1a-b4f170be40b7@oss.nttdata.com>
Date: Wed, 16 Jul 2025 10:39:07 +0900
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Document default values for pgoutput options + fix missing
 initialization for "origin"
From: Fujii Masao <masao.fujii@oss.nttdata.com>
To: Euler Taveira <euler@eulerto.com>, pgsql-hackers@lists.postgresql.org
References: <d2790f10-238d-4cb5-a743-d9d2a9dd900f@oss.nttdata.com>
 <8d21fb98-5c25-4dee-8387-e5a62b01ea7d@app.fastmail.com>
 <a63b5c08-07e5-4fd7-89f9-38f748f4ebc3@oss.nttdata.com>
Content-Language: en-US
In-Reply-To: <a63b5c08-07e5-4fd7-89f9-38f748f4ebc3@oss.nttdata.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/f6970e1b-e2d9-47bf-9c1a-b4f170be40b7%40oss.nttdata.com>
Precedence: bulk


On 2025/05/21 16:27, Fujii Masao wrote:
>=20
>=20
> 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_recvl=
ogical
>>> with pgoutput plugin and needing to know the default behavior of each=
 option.
>>> https://www.postgresql.org/docs/devel/protocol-logical-replication.ht=
ml <https://www.postgresql.org/docs/devel/protocol-logical-replication.ht=
ml>
>>>
>>> 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 sup=
port
>> other ways (true / false / 1 / 0) to write boolean but only document o=
ne way
>> (on / off).
>=20
> Thanks for the review!
>=20
> +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 "Bo=
olean"
>> from streaming option. It is not a boolean anymore since protocol vers=
ion 4.
>> The suggested description is:
>>
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Option to enable streaming of in=
-progress transactions. Valid values are
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 <literal>off</literal> (the defa=
ult), <literal>on</literal> and
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 <literal>parallel</literal>. The=
 setting <literal>parallel</literal>
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 enables sending extra informatio=
n with some messages to be used for
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 parallelization. Minimum protoco=
l version 2 is required to turn it
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 <literal>on</literal>.=C2=A0 Min=
imum protocol version 4 is required for the
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 <literal>parallel</literal> valu=
e.
>=20
> 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 par=
ameters
>>> (like "binary") are explicitly initialized in parse_output_parameters=
(),
>>> the "origin" parameter is not. Its value (PGOutputData->publish_no_or=
igin)
>>> is implicitly set to false due to zero-initialization, but unlike oth=
er
>>> parameters, it lacks an explicit default assignment.
>>>
>>> To ensure consistency and make the behavior clearer, I propose explic=
itly
>>> 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.
>=20
> 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,

--=20
Fujii Masao
NTT DATA Japan Corporation