From Sun, Sep 5, 2021 9:58 PM Masahiko Sawada <sawada.mshk@gmail.com>:
> On Thu, Sep 2, 2021 at 8:37 PM houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com> wrote:
> >
> > From Mon, Aug 30, 2021 3:07 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > I've attached rebased patches. 0004 patch is not the scope of this
> > > patch. It's borrowed from another thread[1] to fix the assertion
> > > failure for newly added tests. Please review them.
> >
> > Hi,
> >
> > I reviewed the 0002 patch and have a suggestion for it.
> @@ -3672,11 +3671,12 @@ typedef enum AlterSubscriptionType typedef
> struct AlterSubscriptionStmt {
> NodeTag type;
> - AlterSubscriptionType kind; /* ALTER_SUBSCRIPTION_SET_OPTIONS,
> etc */
> + AlterSubscriptionType kind; /* ALTER_SUBSCRIPTION_OPTIONS, etc
> + */
> char *subname; /* Name of the subscription */
> char *conninfo; /* Connection string to publisher */
> List *publication; /* One or more publication to
> subscribe to */
> List *options; /* List of DefElem nodes */
> + bool isReset; /* true if RESET option */
> } AlterSubscriptionStmt;
>
> It's unnatural to me that AlterSubscriptionStmt has isReset flag even in spite of
> having 'kind' indicating the command. How about having RESET comand use
> the same logic of SET as you suggested while having both
> ALTER_SUBSCRIPTION_SET_OPTIONS and
> ALTER_SUBSCRIPTION_RESET_OPTIONS?
Yes, I agree with you it will look more natural with ALTER_SUBSCRIPTION_RESET_OPTIONS.
Best regards,
Hou zj