On Fri, Sep 24, 2021 at 5:27 PM Greg Nancarrow <gregn4...@gmail.com> wrote: > > On Tue, Sep 21, 2021 at 2:53 PM Masahiko Sawada <sawada.m...@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/
