Feedback-ID: i0c21471d:Fastmail
MIME-Version: 1.0
Date: Mon, 19 May 2025 23:40:40 -0300
From: "Euler Taveira" <euler@eulerto.com>
To: pgsql-hackers@lists.postgresql.org
Cc: masao.fujii@oss.nttdata.com
Message-Id: <8d21fb98-5c25-4dee-8387-e5a62b01ea7d@app.fastmail.com>
In-Reply-To: <d2790f10-238d-4cb5-a743-d9d2a9dd900f@oss.nttdata.com>
References: <d2790f10-238d-4cb5-a743-d9d2a9dd900f@oss.nttdata.com>
Subject: Re: Document default values for pgoutput options + fix missing
 initialization
 for "origin"
Content-Type: multipart/alternative;
 boundary=ea86e643a0314be7ac7dc88be1800504
Archived-At: 
 <https://www.postgresql.org/message-id/8d21fb98-5c25-4dee-8387-e5a62b01ea7d%40app.fastmail.com>
Precedence: bulk

--ea86e643a0314be7ac7dc88be1800504
Content-Type: text/plain
Content-Transfer-Encoding: 7bit

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.


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


--
Euler Taveira
EDB   https://www.enterprisedb.com/

--ea86e643a0314be7ac7dc88be1800504
Content-Type: text/html
Content-Transfer-Encoding: quoted-printable

<!DOCTYPE html><html><head><title></title></head><body><div>On Fri, May =
16, 2025, at 12:06 PM, Fujii Masao wrote:</div><blockquote type=3D"cite"=
 id=3D"qt" style=3D""><div>The pgoutput plugin options are documented in=
 the logical streaming</div><div>replication protocol, but their default=
 values are not mentioned.</div><div>This can be inconvenient for users =
- for example, when using pg_recvlogical</div><div>with pgoutput plugin =
and needing to know the default behavior of each option.</div><div><a hr=
ef=3D"https://www.postgresql.org/docs/devel/protocol-logical-replication=
.html">https://www.postgresql.org/docs/devel/protocol-logical-replicatio=
n.html</a></div><div><br></div><div>I'd like to propose adding the defau=
lt values to the documentation to</div><div>improve clarity and usabilit=
y. Patch attached (0001 patch).</div></blockquote><div><br></div><div>Go=
od catch.</div><div><br></div><div>Should we use "on" and "off" as other=
 enum GUCs (wal_compression,</div><div>recovery_prefetch, compute_query_=
id)? The current convention is to support</div><div>other ways (true / f=
alse / 1 / 0) to write boolean but only document one way</div><div>(on /=
 off).</div><div><br></div><div>Since you are changing this page, I woul=
d like to suggest removing "Boolean"</div><div>from streaming option. It=
 is not a boolean anymore since protocol version 4.</div><div>The sugges=
ted description is:</div><div><br></div><div>+&nbsp;&nbsp;&nbsp;&nbsp;&n=
bsp;&nbsp; Option to enable streaming of in-progress transactions. Valid=
 values are</div><div>+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; &lt;literal&=
gt;off&lt;/literal&gt; (the default), &lt;literal&gt;on&lt;/literal&gt; =
and</div><div>+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; &lt;literal&gt;paral=
lel&lt;/literal&gt;. The setting &lt;literal&gt;parallel&lt;/literal&gt;=
</div><div>+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; enables sending extra i=
nformation with some messages to be used for</div><div>+&nbsp;&nbsp;&nbs=
p;&nbsp;&nbsp;&nbsp; parallelization. Minimum protocol version 2 is requ=
ired to turn it</div><div>+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; &lt;lite=
ral&gt;on&lt;/literal&gt;.&nbsp; Minimum protocol version 4 is required =
for the</div><div>+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; &lt;literal&gt;p=
arallel&lt;/literal&gt; value.</div><div><br></div><div><br></div><block=
quote type=3D"cite" id=3D"qt" style=3D""><div>While working on this, I a=
lso noticed that although most optional parameters</div><div>(like "bina=
ry") are explicitly initialized in parse_output_parameters(),</div><div>=
the "origin" parameter is not. Its value (PGOutputData-&gt;publish_no_or=
igin)</div><div>is implicitly set to false due to zero-initialization, b=
ut unlike other</div><div>parameters, it lacks an explicit default assig=
nment.</div><div><br></div><div>To ensure consistency and make the behav=
ior clearer, I propose explicitly</div><div>initializing the "origin" pa=
rameter as well. A patch for this is also attached</div><div>(0002 patch=
).</div></blockquote><div><br></div><div>LGTM. It seems an oversight fro=
m the original commit 366283961ac0.</div><div><br></div><div><br></div><=
div id=3D"sig112409080"><div class=3D"signature">--</div><div class=3D"s=
ignature">Euler Taveira</div><div class=3D"signature">EDB&nbsp;&nbsp; <a=
 href=3D"https://www.enterprisedb.com/">https://www.enterprisedb.com/</a=
></div></div><div><br></div></body></html>
--ea86e643a0314be7ac7dc88be1800504--