Re: Skipping logical replication transactions on subscriber side

On Thu, Sep 2, 2021 at 8:37 PM houzj(dot)fnst(at)fujitsu(dot)com
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> 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);

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/