Re: "pgoutput" options missing on documentation

From: Emre Hasegeli <emre(at)hasegeli(dot)com>
To: Peter Smith <smithpb2250(at)gmail(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
Subject: Re: "pgoutput" options missing on documentation
Date: 2023-12-15 13:35:46
Message-ID: CAE2gYzzpoNDdrb6_OnKzdHh9kXmvfyiPnA6yZ=joPJCv8arATg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> 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".

Attachment Content-Type Size
v02-0001-pgoutput-Improve-error-messages-for-options.patch application/octet-stream 8.7 KB
v02-0002-pgoutput-Document-missing-options.patch application/octet-stream 7.8 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andrew Dunstan 2023-12-15 13:42:57 Re: Clean up find_typedefs and add support for Mac
Previous Message Pavel Stehule 2023-12-15 13:19:07 Re: [PATCH] Add --syntax to postgres for SQL syntax checking