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