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
