RE: Slow catchup of 2PC (twophase) transactions on replica in LR

Dear Peter,

Thanks for giving comments! New patch was posted in [1].

> 0.1 General - Patch name
> 
> /SUBSCIRPTION/SUBSCRIPTION/

Fixed.

> ======
> 0.2 General - Apply
> 
> FYI, there are whitespace warnings:
> 
> git
> apply ../patches_misc/v8-0004-Add-force_alter-option-for-ALTER-SUBSCIRPTI
> ON-.-S.patch
> ../patches_misc/v8-0004-Add-force_alter-option-for-ALTER-SUBSCIRPTION-.-
> S.patch:191:
> trailing whitespace.
> # command will abort the prepared transaction and succeed.
> warning: 1 line adds whitespace errors.

I didn't recognize, fixed.

> ======
> 0.3 General - Regression test fails
> 
> The subscription regression tests are not working.
> 
> ok 158       + publication                              1187 ms
> not ok 159   + subscription                              123 ms
> 
> See review comments #4 and #5 below for the reason why.

Yeah, I missed to update the expected result. Fixed.

> ======
> src/sgml/ref/alter_subscription.sgml
> 
> 1.
>       <para>
>        The <literal>two_phase</literal> parameter can only be altered when
> the
> -      subscription is disabled. When altering the parameter from
> <literal>on</literal>
> -      to <literal>off</literal>, the backend process checks prepared
> -      transactions done by the logical replication worker and aborts them.
> +      subscription is disabled. Altering the parameter from
> <literal>on</literal>
> +      to <literal>off</literal> will be failed when there are prepared
> +      transactions done by the logical replication worker. If you want to alter
> +      the parameter forcibly in this case, <literal>force_alter</literal>
> +      option must be set to <literal>on</literal> at the same time. If
> +      specified, the backend process aborts prepared transactions.
>       </para>
> 1a.
> That "will be failed when..." seems strange. Maybe say "will give an
> error when..."
> 
> ~
> 1b.
> Because "force" is a verb, I think true/false is more natural than
> on/off for this new boolean option. e.g. it acts more like a "flag"
> than a "mode". See all the other boolean options in CREATE
> SUBSCRIPTION -- those are mostly all verbs too and are all true/false
> AFAIK.

Fixed, but note that the part was moved.

> 
> ======
> 
> 2. CREATE SUBSCRIPTION
> 
> For my previous review, two comments [v7-0004#2] and [v7-0004#3] were
> not addressed. Kuroda-san wrote:
> Hmm, but the parameter cannot be used for CREATE SUBSCRIPTION. Should
> we modify to accept and add the description in the doc?
> 
> ~
> 
> Yes, that is what I am suggesting. IMO it is odd for the user to be
> able to ALTER a parameter that cannot be included in the CREATE
> SUBSCRIPTION in the first place. AFAIK there are no other parameters
> that behave that way.

Hmm. I felt that this change required the new attribute in pg_subscription system
catalog. Previously I did not like because it contains huge change, but...I tried to do.
New attribute 'subforcealter', and some parts were updated accordingly.

> src/backend/commands/subscriptioncmds.c
> 
> 3. AlterSubscription
> 
> + if (!opts.force_alter)
> + ereport(ERROR,
> + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("cannot alter %s when there are prepared transactions",
> + "two_phase = off"),
> + errhint("Resolve these transactions or set %s at the same time, and
> then try again.",
> + "force_alter = true")));
> 
> I think saying "at the same time" in the hint is unnecessary. Surely
> the user is allowed to set this parameter separately if they want to?
> 
> e.g.
> ALTER SUBSCRIPTION sub SET (force_alter=true);
> ALTER SUBSCRIPTION sub SET (two_phase=off);

Actually, it was correct. Since force_alter was not recorded in the system catalog, it must
be specified at the same time.
Now, we allow to be separated, so removed.

> ======
> src/test/regress/expected/subscription.out
> 
> 4.
> +-- fail - force_alter cannot be set alone
> +ALTER SUBSCRIPTION regress_testsub SET (force_alter = true);
> +ERROR:  force_alter must be specified with two_phase
> 
> This error cannot happen. You removed that error!

Fixed.

> ======
> src/test/subscription/t/099_twophase_added.pl
> 
> 6.
> +# Try altering the two_phase option to "off." The command will fail since there
> +# is a prepared transaction and the force option is not specified.
> +my $stdout;
> +my $stderr;
> +
> +($result, $stdout, $stderr) = $node_subscriber->psql(
> + 'postgres', "ALTER SUBSCRIPTION regress_sub SET (two_phase = off);");
> +ok($stderr =~ /cannot alter two_phase = off when there are prepared
> transactions/,
> + 'ALTER SUBSCRIPTION failed');
> 
> /force option is not specified./'force_alter' option is not specified as true./

Fixed.

> 
> 7.
> +# Verify the prepared transaction still exists
> +$result = $node_subscriber->safe_psql('postgres',
> +    "SELECT count(*) FROM pg_prepared_xacts;");
> +is($result, q(1), "prepared transaction still exits");
> +
> 
> TYPO: /exits/exists/

Fixed.

> 
> ~~~
> 
> 8.
> +# Alter the two_phase with the force_alter option. Apart from the above, the
> +# command will abort the prepared transaction and succeed.
> +$node_subscriber->safe_psql('postgres',
> +    "ALTER SUBSCRIPTION regress_sub SET (two_phase = off, force_alter
> = true);");
> +$node_subscriber->safe_psql('postgres', "ALTER SUBSCRIPTION
> regress_sub ENABLE;");
> +
> 
> What does "Apart from the above" mean? Be more explicit.

Clarified like "Apart from the last ALTER SUBSCRIPTION command...".

> 9.
> +# Verify the prepared transaction are aborted
>  $result = $node_subscriber->safe_psql('postgres',
>      "SELECT count(*) FROM pg_prepared_xacts;");
>  is($result, q(0), "prepared transaction done by worker is aborted");
> 
> /transaction are aborted/transaction was aborted/

Fixed.

[1]:
https://www.postgresql.org/message-id/OSBPR01MB2552FEA48D265EA278AA9F7AF5E22%40OSBPR01MB2552.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/