Re: "pgoutput" options missing on documentation
On Thu, Dec 21, 2023 at 7:16 PM Emre Hasegeli wrote: > > Fixed versions are attached. > Pushed! -- With Regards, Amit Kapila.
Re: "pgoutput" options missing on documentation
> But the xref seems present only in the master/v16/v15 patches, but not > for the earlier patches v14/v13/v12. Why not? I missed it. > But the change was only in the patches v14 onwards. Although the new > error message was only added for HEAD, isn't it still correct to say > "A valid version is required." for all the patches including v12 and > v13? Yes, it's still correct. Fixed versions are attached. v12-0001-doc-Clarify-pgoutput-options.patch Description: Binary data v14-0001-doc-Clarify-pgoutput-options.patch Description: Binary data v13-0001-doc-Clarify-pgoutput-options.patch Description: Binary data
Re: "pgoutput" options missing on documentation
On Thu, Dec 21, 2023 at 2:58 AM Emre Hasegeli wrote: > > > 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. Hi, I checked (just by visual inspection and diffs) the provided backpatches and I have a couple of questions: == 1. The Chapter "Streaming Replication Protocol" START_REPLICATION / "option_name" part has an xref to the pgoutput options page e.g. master - The name of an option passed to the slot's logical decoding plugin. + The name of an option passed to the slot's logical decoding output + plugin. See for + options that are accepted by the standard (pgoutput) + plugin. But the xref seems present only in the master/v16/v15 patches, but not for the earlier patches v14/v13/v12. Why not? ~~~ 2. The proto_version part now says "A valid version is required.". e.g. master - 3, and 4 are supported. + 3, and 4 are supported. A valid + version is required. But the change was only in the patches v14 onwards. Although the new error message was only added for HEAD, isn't it still correct to say "A valid version is required." for all the patches including v12 and v13? == Kind Regards, Peter Smith. Fujitsu Australia
Re: "pgoutput" options missing on documentation
> 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. v13-0001-doc-Clarify-pgoutput-options.patch Description: Binary data master-0001-doc-Clarify-pgoutput-options.patch Description: Binary data v15-0001-doc-Clarify-pgoutput-options.patch Description: Binary data v12-0001-doc-Clarify-pgoutput-options.patch Description: Binary data v14-0001-doc-Clarify-pgoutput-options.patch Description: Binary data v16-0001-doc-Clarify-pgoutput-options.patch Description: Binary data
Re: "pgoutput" options missing on documentation
On Tue, Dec 19, 2023 at 12:55 PM Amit Kapila 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.
Re: "pgoutput" options missing on documentation
On Tue, Dec 19, 2023 at 6:25 PM Amit Kapila wrote:
>
> On Tue, Dec 19, 2023 at 12:07 PM Peter Smith wrote:
> >
> > On Tue, Dec 19, 2023 at 1:35 AM Emre Hasegeli 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
Re: "pgoutput" options missing on documentation
On Tue, Dec 19, 2023 at 12:07 PM Peter Smith wrote: > > On Tue, Dec 19, 2023 at 1:35 AM Emre Hasegeli 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.
Re: "pgoutput" options missing on documentation
On Tue, Dec 19, 2023 at 1:35 AM Emre Hasegeli 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.
~~~
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
Kind Regards,
Peter Smith.
Fujitsu Australia
parse_output_parameters.diff
Description: Binary data
Re: "pgoutput" options missing on documentation
> 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.
Re: "pgoutput" options missing on documentation
On Mon, Dec 18, 2023 at 1:08 PM Emre Hasegeli 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.
Re: "pgoutput" options missing on documentation
> 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 supports extensible logical decoding plugin > pgoutput 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. v03-0002-doc-Clarify-pgoutput-options.patch Description: Binary data v03-0001-pgoutput-Test-if-proto_version-is-given.patch Description: Binary data
Re: "pgoutput" options missing on documentation
> 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.
Re: "pgoutput" options missing on documentation
On Fri, Dec 15, 2023 at 7:06 PM Emre Hasegeli 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: + + PostgreSQL supports extensible logical decoding + plugins. pgoutput is the standard one used for + the built-in logical replication. + This sounds like we are supporting more than one logical decoding plugin. Can we slightly rephrase it to something like: "PostgreSQL supports extensible logical decoding plugin pgoutput which is used for the built-in logical replication as well." -- With Regards, Amit Kapila.
Re: "pgoutput" options missing on documentation
On Fri, Dec 15, 2023 at 7:06 PM Emre Hasegeli 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.
Re: "pgoutput" options missing on documentation
On Mon, Dec 18, 2023 at 11:35 AM Peter Smith wrote: > > Hi, I had a look at the latest v02 patches. > > On Sat, Dec 16, 2023 at 12:36 AM Emre Hasegeli 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 >= 16 ? LOGICALREP_PROTO_STREAM_PARALLEL_VERSION_NUM : server_version >= 15 ? LOGICALREP_PROTO_TWOPHASE_VERSION_NUM : server_version >= 14 ? LOGICALREP_PROTO_STREAM_VERSION_NUM : LOGICALREP_PROTO_VERSION_NUM; == Kind Regards, Peter Smith. Fujitsu Australia
Re: "pgoutput" options missing on documentation
Hi, I had a look at the latest v02 patches. On Sat, Dec 16, 2023 at 12:36 AM Emre Hasegeli 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
Re: "pgoutput" options missing on documentation
> 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 for details about options that are accepted > by the standard (pgoutput) plugin. Good idea. Incorporated. > 3. > > - The logical replication START_REPLICATION command > - accepts following parameters: > + Using the START_REPLICATION command, > + pgoutput) accepts the following options: > > > Oops, you copied my typo. There is a spurious ')'. Fixed. > 4. > + > + > + > + origin > + > + > + > + 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. > + > + > + > > > 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". v02-0001-pgoutput-Improve-error-messages-for-options.patch Description: Binary data v02-0002-pgoutput-Document-missing-options.patch Description: Binary data
Re: "pgoutput" options missing on documentation
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 for details about options that are accepted
by the standard (pgoutput) plugin.
~~~
3.
- The logical replication START_REPLICATION command
- accepts following parameters:
+ Using the START_REPLICATION command,
+ pgoutput) accepts the following options:
Oops, you copied my typo. There is a spurious ')'.
~~~
4.
+
+
+
+ origin
+
+
+
+ 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.
+
+
+
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
Re: "pgoutput" options missing on documentation
> 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.
Re: "pgoutput" options missing on documentation
> 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 START_REPLICATION command, > pgoutput) 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. v01-0001-pgoutput-Improve-error-messages-for-options.patch Description: Binary data v01-0002-pgoutput-Document-missing-options.patch Description: Binary data
Re: "pgoutput" options missing on documentation
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 START_REPLICATION command
- accepts following parameters:
+ The standard logical decoding plugin (pgoutput) accepts
+ following parameters with START_REPLICATION command:
Since the previous line already said pgoutput is the standard decoding
plugin, maybe it's not necessary to repeat that.
SUGGESTION
Using the START_REPLICATION command,
pgoutput) 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
Re: "pgoutput" options missing on documentation
On Wed, Dec 13, 2023 at 9:25 PM Emre Hasegeli 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.
"pgoutput" options missing on documentation
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. [1] https://www.postgresql.org/docs/devel/protocol-logical-replication.html v00-0001-pgoutput-Test-if-protocol_version-is-given.patch Description: Binary data v00-0002-pgoutput-Document-missing-options.patch Description: Binary data
