RE: Skipping logical replication transactions on subscriber side

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.

+                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