Re: Skipping logical replication transactions on subscriber side

On Fri, Sep 10, 2021 at 12:33 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> Sorry for the late response. I've attached the updated patches that
> incorporate all comments unless I missed something. Please review
> them.
>

A few review comments for the v13-0002 patch:

(1)
I suggest a small update to the patch comment:

BEFORE:
ALTER SUBSCRIPTION ... RESET command resets subscription
parameters. The parameters that can be set are streaming, binary,
synchronous_commit.
AFTER:
ALTER SUBSCRIPTION ... RESET command resets subscription
parameters to their default value. The parameters that can be reset
are streaming, binary, and synchronous_commit.


(2)
In the documentation, the RESETable parameters should be listed in the
same way and order as for SET:

BEFORE:
+     <para>
+       The parameters that can be reset are: <literal>streaming</literal>,
+       <literal>binary</literal>, <literal>synchronous_commit</literal>.
+     </para>
AFTER:
+     <para>
+       The parameters that can be reset are
<literal>synchronous_commit</literal>,
+       <literal>binary</literal>, and <literal>streaming</literal>.
+      </para>


Also I'm thinking it would be beneficial to say before this:

RESET is used to set parameters back to their default value.

(3)
I notice that if you try to reset the slot_name, you get the following message:

postgres=# alter subscription sub reset (slot_name);
ERROR:  unrecognized subscription parameter: "slot_name"

This is a bit misleading, because slot_name IS a subscription
parameter, just not resettable.
It would be better if it said something like: ERROR: not a resettable
subscription parameter: "slot_name"

However, it seems that this is also an existing issue with SET (e.g.
for "refresh" or "two_phase")
postgres=# alter subscription sub set (refresh=true);
ERROR:  unrecognized subscription parameter: "refresh"


Regards,
Greg Nancarrow
Fujitsu Australia