On Fri, Sep 24, 2021 at 5:27 PM Greg Nancarrow <gregn4422@gmail.com> wrote:
>
> On Tue, Sep 21, 2021 at 2:53 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > I've attached the updated version patches. Please review them.
> >
>
> A few review comments for the v14-0002 patch:
Thank you for the comments!
>
> (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 the following before this:
>
> RESET is used to set parameters back to their default value.
>
I agreed with all of the above comments. I'll incorporate them into
the next version patch that I'm going to submit next Monday.
> (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" actually IS a
> subscription parameter, just not resettable.
> It would be better in this case 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"
Good point. Maybe we can improve it in a separate patch?
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
