On Fri, Sep 10, 2021 at 12:33 AM Masahiko Sawada <sawada.m...@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
