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