On Thu, Sep 2, 2021 at 8:37 PM houzj.f...@fujitsu.com <houzj.f...@fujitsu.com> wrote: > > From Mon, Aug 30, 2021 3:07 PM Masahiko Sawada <sawada.m...@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. > > + if (IsSet(opts.specified_opts, > SUBOPT_SYNCHRONOUS_COMMIT)) > + { > + > values[Anum_pg_subscription_subsynccommit - 1] = > + CStringGetTextDatum("off"); > + > replaces[Anum_pg_subscription_subsynccommit - 1] = true; > + } > > Currently, the patch set the default value out of > parse_subscription_options(), > but I think It might be more standard to set the value in > parse_subscription_options(). Like: > > if (!is_reset) > { > ... > + } > + else > + opts->synchronous_commit = "off"; > > And then, we can set the value like: > > > values[Anum_pg_subscription_subsynccommit - 1] = > > CStringGetTextDatum(opts.synchronous_commit);
You're right. Fixed. > > > Besides, instead of adding a switch case like the following: > + case ALTER_SUBSCRIPTION_RESET_OPTIONS: > + { > > We can add a bool flag(isReset) in AlterSubscriptionStmt and check the flag > when invoking parse_subscription_options(). In this approach, the code can be > shorter. > > Attach a diff file based on the v12-0002 which change the code like the above > suggestion. Thank you for the patch! @@ -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? Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/