RE: Skipping logical replication transactions on subscriber side

From Mon, Aug 30, 2021 3:07 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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