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);
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.
Best regards,
Hou zj
0001-diff-for-0002_patch
Description: 0001-diff-for-0002_patch
