"pgoutput" options missing on documentation

On Wed, Dec 13, 2023 at 9:25 PM Emre Hasegeli <emre@hasegeli.com> wrote:

I noticed that Logical Streaming Replication Protocol documentation
[1] is missing the options added to "pgoutput" since version 14. A
patch is attached to fix it together with another small one to give a
nice error when "proto_version" parameter is not provided.

I agree that we missed updating the parameters of the Logical
Streaming Replication Protocol documentation. I haven't reviewed all
the details yet but one minor thing that caught my attention while
looking at your patch is that we can update the exact additional
information we started to send for streaming mode parallel. We should
update the following sentence: "It accepts an additional value
"parallel" to enable sending extra information with the "Stream Abort"
messages to be used for parallelisation."

--
With Regards,
Amit Kapila.

Hi, here are some initial review comments.

//////

Patch v00-0001

1.
+
+ /* Check required parameters */
+ if (!protocol_version_given)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("proto_version parameter missing")));
+ if (!publication_names_given)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("publication_names parameter missing")));

To reduce translation efforts, perhaps it is better to arrange for
these to share a common message.

For example,

ereport(ERROR,
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
/* translator: %s is a pgoutput option */
errmsg("missing pgoutput option: %s", "proto_version"));

~

ereport(ERROR,
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
/* translator: %s is a pgoutput option */
errmsg("missing pgoutput option: %s", "publication_names"));

Also, I am unsure whether to call these "parameters" or "options" -- I
wanted to call them parameters like in the documentation, but every
other message in this function refers to "options", so in my example,
I mimicked the nearby code YMMV.

//////

Patch v00-0002

2.
-   The logical replication <literal>START_REPLICATION</literal> command
-   accepts following parameters:
+   The standard logical decoding plugin (<literal>pgoutput</literal>) accepts
+   following parameters with <literal>START_REPLICATION</literal> command:

Since the previous line already said pgoutput is the standard decoding
plugin, maybe it's not necessary to repeat that.

SUGGESTION
Using the <literal>START_REPLICATION</literal> command,
<literal>pgoutput</literal>) accepts the following parameters:

~~~

3.
I noticed in the protocol message formats docs [1]https://www.postgresql.org/docs/current/protocol-logicalrep-message-formats.html that those messages
are grouped according to the protocol version that supports them.
Perhaps this page should be arranged similarly for these parameters?

For example, document the parameters in the order they were introduced.

SUGGESTION

-proto_version
...
-publication_names
...
-binary
...
-messages
...

Since protocol version 2:

-streaming (boolean)
...

Since protocol version 3:

-two_phase
...

Since protocol version 4:

-streaming (boolean/parallel)
...
-origin
...

~~~

4.
+       Boolean parameter to use the binary transfer mode.  This is faster
+       than the text mode, but slightly less robust

SUGGESTION
Boolean parameter to use binary transfer mode. Binary mode is faster
than the text mode but slightly less robust

======
[1]: https://www.postgresql.org/docs/current/protocol-logicalrep-message-formats.html

Kind Regards,
Peter Smith.
Fujitsu Australia

To reduce translation efforts, perhaps it is better to arrange for
these to share a common message.

Good idea. I've done so.

Also, I am unsure whether to call these "parameters" or "options" -- I
wanted to call them parameters like in the documentation, but every
other message in this function refers to "options", so in my example,
I mimicked the nearby code YMMV.

It looks like they are called "options" in most places. I changed the
documentation to be consistent too.

Since the previous line already said pgoutput is the standard decoding
plugin, maybe it's not necessary to repeat that.

SUGGESTION
Using the <literal>START_REPLICATION</literal> command,
<literal>pgoutput</literal>) accepts the following parameters:

Changed.

3.
I noticed in the protocol message formats docs [1] that those messages
are grouped according to the protocol version that supports them.
Perhaps this page should be arranged similarly for these parameters?

For example, document the parameters in the order they were introduced.

SUGGESTION

-proto_version
...
-publication_names
...
-binary
...
-messages
...

Since protocol version 2:

-streaming (boolean)
...

Since protocol version 3:

-two_phase
...

Since protocol version 4:

-streaming (boolean/parallel)
...
-origin

This is not going to be correct because not all options do require a
protocol version. "origin" is added in version 16, but doesn't check
for any "proto_version". Perhaps we should fix this too.

4.
+       Boolean parameter to use the binary transfer mode.  This is faster
+       than the text mode, but slightly less robust

SUGGESTION
Boolean parameter to use binary transfer mode. Binary mode is faster
than the text mode but slightly less robust

Done.

Thanks for the review.

The new versions are attached.

I agree that we missed updating the parameters of the Logical
Streaming Replication Protocol documentation. I haven't reviewed all
the details yet but one minor thing that caught my attention while
looking at your patch is that we can update the exact additional
information we started to send for streaming mode parallel. We should
update the following sentence: "It accepts an additional value
"parallel" to enable sending extra information with the "Stream Abort"
messages to be used for parallelisation."

I changed this in the new version.

Thank you for picking this up.

Thanks for the update. Here are some more review comments for the v01* patches.

//////

Patch v00-0001

v01 modified the messages more than I was expecting, although what you
did looks fine to me.

~~~

1.
+ /* Check required options */
+ if (!protocol_version_given)
+ ereport(ERROR,
+ errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ /* translator: %s is a pgoutput option */
+ errmsg("missing pgoutput option: %s", "proto_version"));
+ if (!publication_names_given)
+ ereport(ERROR,
+ errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ /* translator: %s is a pgoutput option */
+ errmsg("missing pgoutput option: %s", "publication_names"));

I saw that the original "publication_names" error was using
errcode(ERRCODE_INVALID_PARAMETER_VALUE), but TBH since there is no
option given at all I felt ERRCODE_SYNTAX_ERROR might be more
appropriate errcode for those 2 mandatory option errors.

//////

Patch v00-0002

2.

I saw that the chapter "55.4. Streaming Replication Protocol" [1]55.4 https://www.postgresql.org/docs/devel/protocol-replication.html
introduces "START_REPLICATION SLOT slot_name LOGICAL ..." command and
it says
---
option_name
The name of an option passed to the slot's logical decoding plugin.
---

Perhaps that part should now include a reference to your new information:

SUGGESTION
option_name
The name of an option passed to the slot's logical decoding plugin.
Please see <XXX (55.5.1)> for details about options that are accepted
by the standard (pgoutput) plugin.

~~~

3.
   <para>
-   The logical replication <literal>START_REPLICATION</literal> command
-   accepts following parameters:
+   Using the <literal>START_REPLICATION</literal> command,
+   <literal>pgoutput</literal>) accepts the following options:

Oops, you copied my typo. There is a spurious ')'.

~~~

4.
+<!-- Backpack through version 16. -->
+    <varlistentry>
+     <term>
+      origin
+     </term>
+     <listitem>
+      <para>
+       String option to send only changes by an origin.  It also gets
+       the option "none" to send the changes that have no origin associated,
+       and the option "any" to send the changes regardless of their origin.
+       This can be used to avoid loops (infinite replication of the same data)
+       among replication nodes.
+      </para>
+     </listitem>
+    </varlistentry>
    </variablelist>

AFAIK pgoutput only understands origin values "any" and "none" and
nothing else; I felt the "It also gets..." part implies it is more
flexible than it is.

SUGGESTION
Possible values are "none" (to only send the changes that have no
origin associated), or "any" (to send the changes regardless of their
origin).

~~~

5. Rearranging option details

SUGGESTION

-proto_version
...
-publication_names
...
-binary
...
-messages
...

Since protocol version 2:

-streaming (boolean)
...

Since protocol version 3:

-two_phase
...

Since protocol version 4:

-streaming (boolean/parallel)
...
-origin

This is not going to be correct because not all options do require a
protocol version. "origin" is added in version 16, but doesn't check
for any "proto_version". Perhaps we should fix this too.

OK, to deal with that can't you just include "origin" in the first
group which has no special protocol requirements?

SUGGESTION
-proto_version
-publication_names
-binary
-messages
-origin

Requires minimum protocol version 2:
-streaming (boolean)

Requires minimum protocol version 3:
-two_phase

Requires minimum protocol version 4:
-streaming (parallel)

======
[1]: 55.4 https://www.postgresql.org/docs/devel/protocol-replication.html

Kind Regards,
Peter Smith.
Fujitsu Australia

I saw that the original "publication_names" error was using
errcode(ERRCODE_INVALID_PARAMETER_VALUE), but TBH since there is no
option given at all I felt ERRCODE_SYNTAX_ERROR might be more
appropriate errcode for those 2 mandatory option errors.

It makes sense to me. Changed.

2.

I saw that the chapter "55.4. Streaming Replication Protocol" [1]
introduces "START_REPLICATION SLOT slot_name LOGICAL ..." command and
it says
---
option_name
The name of an option passed to the slot's logical decoding plugin.
---

Perhaps that part should now include a reference to your new information:

SUGGESTION
option_name
The name of an option passed to the slot's logical decoding plugin.
Please see <XXX (55.5.1)> for details about options that are accepted
by the standard (pgoutput) plugin.

Good idea. Incorporated.

3.
<para>
-   The logical replication <literal>START_REPLICATION</literal> command
-   accepts following parameters:
+   Using the <literal>START_REPLICATION</literal> command,
+   <literal>pgoutput</literal>) accepts the following options:

Oops, you copied my typo. There is a spurious ')'.

Fixed.

4.
+<!-- Backpack through version 16. -->
+    <varlistentry>
+     <term>
+      origin
+     </term>
+     <listitem>
+      <para>
+       String option to send only changes by an origin.  It also gets
+       the option "none" to send the changes that have no origin associated,
+       and the option "any" to send the changes regardless of their origin.
+       This can be used to avoid loops (infinite replication of the same data)
+       among replication nodes.
+      </para>
+     </listitem>
+    </varlistentry>
</variablelist>

AFAIK pgoutput only understands origin values "any" and "none" and
nothing else; I felt the "It also gets..." part implies it is more
flexible than it is.

SUGGESTION
Possible values are "none" (to only send the changes that have no
origin associated), or "any" (to send the changes regardless of their
origin).

Oh, it's not how I understood it. I think you are right. Changed.

OK, to deal with that can't you just include "origin" in the first
group which has no special protocol requirements?

I think it'd be confusing because the option is not available before
version 16. I think it should really check for the version number and
complain if it's less than 4.

SUGGESTION
-proto_version
-publication_names
-binary
-messages
-origin

Requires minimum protocol version 2:
-streaming (boolean)

Requires minimum protocol version 3:
-two_phase

Requires minimum protocol version 4:
-streaming (parallel)

I am still not sure if this is any better. I don't like that
"streaming" appears twice, and I wouldn't know how to format this
nicely.

The new versions are attached.

I also added "Required." for "proto_version" and "publication_names".

Hi, I had a look at the latest v02 patches.

On Sat, Dec 16, 2023 at 12:36 AM Emre Hasegeli <emre@hasegeli.com> wrote:

OK, to deal with that can't you just include "origin" in the first
group which has no special protocol requirements?

I think it'd be confusing because the option is not available before
version 16. I think it should really check for the version number and
complain if it's less than 4.

Hm. I don't think a proto_version check is required for "origin".

IIUC, the protocol version number indicates the message byte format.
It's needed so that those messages bytes can be read/written in the
same/compatible way. OTOH I thought the "origin" option has nothing
really to do with actual message formats on the wire; I think it works
just by filtering up-front to decide either to send the changes or not
send the changes. For example, so long as PostgreSQL >= v16, I expect
you could probably use "origin" with any proto_version you wanted.

SUGGESTION
-proto_version
-publication_names
-binary
-messages
-origin

Requires minimum protocol version 2:
-streaming (boolean)

Requires minimum protocol version 3:
-two_phase

Requires minimum protocol version 4:
-streaming (parallel)

I am still not sure if this is any better. I don't like that
"streaming" appears twice, and I wouldn't know how to format this
nicely.

I won't keep pushing to rearrange the docs. I think all the content is
OK anyway, so let's see if other people have any opinions on how the
new information is best presented.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Mon, Dec 18, 2023 at 11:35 AM Peter Smith <smithpb2250@gmail.com> wrote:

Hi, I had a look at the latest v02 patches.

On Sat, Dec 16, 2023 at 12:36 AM Emre Hasegeli <emre@hasegeli.com> wrote:

OK, to deal with that can't you just include "origin" in the first
group which has no special protocol requirements?

I think it'd be confusing because the option is not available before
version 16. I think it should really check for the version number and
complain if it's less than 4.

Hm. I don't think a proto_version check is required for "origin".

IIUC, the protocol version number indicates the message byte format.
It's needed so that those messages bytes can be read/written in the
same/compatible way. OTOH I thought the "origin" option has nothing
really to do with actual message formats on the wire; I think it works
just by filtering up-front to decide either to send the changes or not
send the changes. For example, so long as PostgreSQL >= v16, I expect
you could probably use "origin" with any proto_version you wanted.

But, I don't know how the user would be able to arrange for such a
mixture of PG/proto_version versions. because they do seem tightly
coupled for pgoutput.

e.g.
server_version = walrcv_server_version(LogRepWorkerWalRcvConn);
options.proto.logical.proto_version =
server_version >= 160000 ?
LOGICALREP_PROTO_STREAM_PARALLEL_VERSION_NUM :
server_version >= 150000 ? LOGICALREP_PROTO_TWOPHASE_VERSION_NUM :
server_version >= 140000 ? LOGICALREP_PROTO_STREAM_VERSION_NUM :
LOGICALREP_PROTO_VERSION_NUM;

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Fri, Dec 15, 2023 at 7:06 PM Emre Hasegeli <emre@hasegeli.com> wrote:

I saw that the original "publication_names" error was using
errcode(ERRCODE_INVALID_PARAMETER_VALUE), but TBH since there is no
option given at all I felt ERRCODE_SYNTAX_ERROR might be more
appropriate errcode for those 2 mandatory option errors.

It makes sense to me. Changed.

I found the existing error code appropriate because for syntax
specification, either we need to mandate this at the grammar level or
at the API level. Also, I think we should give a message similar to an
existing message: "publication_names parameter missing". For example,
we can say, "proto_version parameter missing". BTW, I also don't like
the other changes parse_output_parameters() done in 0001, if we want
to improve all the similar messages there are other places in the code
as well, so we can separately make the case for the same.

--
With Regards,
Amit Kapila.

On Fri, Dec 15, 2023 at 7:06 PM Emre Hasegeli <emre@hasegeli.com> wrote:

SUGGESTION
-proto_version
-publication_names
-binary
-messages
-origin

Requires minimum protocol version 2:
-streaming (boolean)

Requires minimum protocol version 3:
-two_phase

Requires minimum protocol version 4:
-streaming (parallel)

I am still not sure if this is any better. I don't like that
"streaming" appears twice, and I wouldn't know how to format this
nicely.

The currently proposed way seems reasonable to me.

The new versions are attached.

I also added "Required." for "proto_version" and "publication_names".

Comma separated list of publication names for which to subscribe
-       (receive changes). The individual publication names are treated
-       as standard objects names and can be quoted the same as needed.
+       (receive changes).  Required.  The individual publication names are

This change (Required in between two sentences) looks slightly odd to
me. Can we instead extend the second line to something like: "This
parameter is required, and the individual publication names are ...".
Similarly we can adjust the proto_vesion explanation.

One minor comment:
====================
+ <para>
+  <productname>PostgreSQL</productname> supports extensible logical decoding
+  plugins.  <literal>pgoutput</literal> is the standard one used for
+  the built-in logical replication.
+ </para>

This sounds like we are supporting more than one logical decoding
plugin. Can we slightly rephrase it to something like:
"PostgreSQL</productname> supports extensible logical decoding plugin
<literal>pgoutput</literal> which is used for the built-in logical
replication as well."

--
With Regards,
Amit Kapila.

I found the existing error code appropriate because for syntax
specification, either we need to mandate this at the grammar level or
at the API level. Also, I think we should give a message similar to an
existing message: "publication_names parameter missing". For example,
we can say, "proto_version parameter missing". BTW, I also don't like
the other changes parse_output_parameters() done in 0001, if we want
to improve all the similar messages there are other places in the code
as well, so we can separately make the case for the same.

Okay, I am changing these back. I think we should keep the word
"option". It is used on other error messages.

This change (Required in between two sentences) looks slightly odd to
me. Can we instead extend the second line to something like: "This
parameter is required, and the individual publication names are ...".
Similarly we can adjust the proto_vesion explanation.

I don't think it's an improvement to join 2 independent sentences with
a comma. I expanded these by mentioning what is required.

This sounds like we are supporting more than one logical decoding
plugin. Can we slightly rephrase it to something like:
"PostgreSQL</productname> supports extensible logical decoding plugin
<literal>pgoutput</literal> which is used for the built-in logical
replication as well."

I understand the confusion. I reworded it and dropped "extensible".

The new versions are attached.

On Mon, Dec 18, 2023 at 1:08 PM Emre Hasegeli <emre@hasegeli.com> wrote:

I found the existing error code appropriate because for syntax
specification, either we need to mandate this at the grammar level or
at the API level. Also, I think we should give a message similar to an
existing message: "publication_names parameter missing". For example,
we can say, "proto_version parameter missing". BTW, I also don't like
the other changes parse_output_parameters() done in 0001, if we want
to improve all the similar messages there are other places in the code
as well, so we can separately make the case for the same.

Okay, I am changing these back. I think we should keep the word
"option". It is used on other error messages.

Fair enough. I think we should push your first patch only in HEAD as
this is a minor improvement over the current behaviour. What do you
think?

--
With Regards,
Amit Kapila.

Fair enough. I think we should push your first patch only in HEAD as
this is a minor improvement over the current behaviour. What do you
think?

I agree.

On Tue, Dec 19, 2023 at 1:35 AM Emre Hasegeli <emre@hasegeli.com> wrote:

Fair enough. I think we should push your first patch only in HEAD as
this is a minor improvement over the current behaviour. What do you
think?

I agree.

Patch 0001

AFAICT parse_output_parameters possible errors are never tested. For
example, there is no code coverage [1]code coverage -- https://coverage.postgresql.org/src/backend/replication/pgoutput/pgoutput.c.gcov.html touching any of these ereports.

IMO there should be some simple test cases -- I am happy to create
some tests if you agree they should exist.

~~~

While looking at the function parse_output_parameters() I noticed that
if an unrecognised option is passed the function emits an elog instead
of an ereport

------
test_pub=# SELECT * FROM pg_logical_slot_get_changes('test_slot_v1',
NULL, NULL, 'banana', '1');
2023-12-19 17:08:21.627 AEDT [8921] ERROR: unrecognized pgoutput option: banana
2023-12-19 17:08:21.627 AEDT [8921] CONTEXT: slot "test_slot_v1",
output plugin "pgoutput", in the startup callback
2023-12-19 17:08:21.627 AEDT [8921] STATEMENT: SELECT * FROM
pg_logical_slot_get_changes('test_slot_v1', NULL, NULL, 'banana',
'1');
ERROR: unrecognized pgoutput option: banana
CONTEXT: slot "test_slot_v1", output plugin "pgoutput", in the startup callback
------

But that doesn't seem right. AFAIK elog messages use errmsg_internal
so this message would not get translated.

PSA a patch to fix that.

======
[1]: code coverage -- https://coverage.postgresql.org/src/backend/replication/pgoutput/pgoutput.c.gcov.html
https://coverage.postgresql.org/src/backend/replication/pgoutput/pgoutput.c.gcov.html

Kind Regards,
Peter Smith.
Fujitsu Australia

On Tue, Dec 19, 2023 at 12:07 PM Peter Smith <smithpb2250@gmail.com> wrote:

On Tue, Dec 19, 2023 at 1:35 AM Emre Hasegeli <emre@hasegeli.com> wrote:

Fair enough. I think we should push your first patch only in HEAD as
this is a minor improvement over the current behaviour. What do you
think?

I agree.

Patch 0001

AFAICT parse_output_parameters possible errors are never tested. For
example, there is no code coverage [1] touching any of these ereports.

IMO there should be some simple test cases -- I am happy to create
some tests if you agree they should exist.

I don't think having tests for all sorts of error checking will add
much value as compared to the overhead they bring.

~~~

While looking at the function parse_output_parameters() I noticed that
if an unrecognised option is passed the function emits an elog instead
of an ereport

We don't expect unrecognized option here and for such a thing, we use
elog in the code. See the similar usage in
parseCreateReplSlotOptions().

I think we should move to 0002 patch now. In that, I suggest preparing
separate back branch patches.

--
With Regards,
Amit Kapila.

On Tue, Dec 19, 2023 at 6:25 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Dec 19, 2023 at 12:07 PM Peter Smith <smithpb2250@gmail.com> wrote:

On Tue, Dec 19, 2023 at 1:35 AM Emre Hasegeli <emre@hasegeli.com> wrote:

Fair enough. I think we should push your first patch only in HEAD as
this is a minor improvement over the current behaviour. What do you
think?

I agree.

Patch 0001

AFAICT parse_output_parameters possible errors are never tested. For
example, there is no code coverage [1] touching any of these ereports.

IMO there should be some simple test cases -- I am happy to create
some tests if you agree they should exist.

I don't think having tests for all sorts of error checking will add
much value as compared to the overhead they bring.

~~~

While looking at the function parse_output_parameters() I noticed that
if an unrecognised option is passed the function emits an elog instead
of an ereport

We don't expect unrecognized option here and for such a thing, we use
elog in the code. See the similar usage in
parseCreateReplSlotOptions().

IIUC the untranslated elog should be used for internal/sanity errors,
debugging, or stuff that cannot happen under any normal circumstances.
While that may be the case for parseCreateReplSlotOptions() mentioned,
IMO the scenario in the parse_output_parameters() is very different,
because these options can come directly from user input so any user
typo can cause this error. Indeed, this is probably one of the more
likely reasons for getting any error in parse_output_parameters()
function. I thought any errors that can be easily caused by some user
actions ought to be translated.

For example, the user accidentally misspells 'proto_version':

test_pub=# SELECT * FROM pg_logical_slot_get_changes('test_slot_v1',
NULL, NULL, 'protocol_version', '1');
ERROR: unrecognized pgoutput option: protocol_version
CONTEXT: slot "test_slot_v1", output plugin "pgoutput", in the startup callback

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Tue, Dec 19, 2023 at 12:55 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

I think we should move to 0002 patch now. In that, I suggest preparing
separate back branch patches.

Emre, are you planning to share back-branch patches?

--
With Regards,
Amit Kapila.

We don't expect unrecognized option here and for such a thing, we use
elog in the code. See the similar usage in
parseCreateReplSlotOptions().

"pgoutput" is useful for a lot of applications other than our logical
replication subscriber. I think we should expect anything and handle
errors nicely.

I think we should move to 0002 patch now. In that, I suggest preparing
separate back branch patches.

They are attached.