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

Dear Peter,

Thanks for reviewing! Here are new patches.

 > 
> //////////
> patch v10-0002
> //////////
> 
> ======
> Commit message
> 
> 2.1.
> Regarding the false->true case, the backend process alters the subtwophase to
> LOGICALREP_TWOPHASE_STATE_PENDING once. After the subscription is
> enabled, a new
> logical replication worker requests to change the two_phase option of its slot
> from pending to true after the initial data synchronization is done. The code
> path is the same as the case in which two_phase is initially set to true, so
> there is no need to do something remarkable. However, for the true->false case,
> the backend must connect to the publisher and expressly change the parameter
> because the apply worker does not alter the option to false. The
> operation cannot
> be rolled back, and altering the parameter from "true" to "false" within a
> transaction is prohibited.
> 
> ~
> 
> BEFORE
> The operation cannot be rolled back, and altering the parameter from
> "true" to "false" within a transaction is prohibited.
> 
> SUGGESTION
> Because this operation cannot be rolled back, altering the two_phase
> parameter from "true" to "false" within a transaction is prohibited.

Fixed.

> 
> ======
> doc/src/sgml/ref/alter_subscription.sgml
> 
> 2.2.
>     <command>ALTER SUBSCRIPTION ... SET (failover = on|off)</command>
> and
> -   <command>ALTER SUBSCRIPTION ... SET (two_phase =
> on|off)</command>
> +   <command>ALTER SUBSCRIPTION ... SET (two_phase = off)</command>
> 
> I wasn't sure why you chose to keep on|off here instead of true|false,
> since in subsequence patch 0003 you changed it true/false everywhere
> as discussed in previous reviews.
> 
> OTOH if you only did this to be consistent with the "failover=on|off"
> then that is OK; but in that case I might raise a separate hackers
> thread to propose those should also be changed to true|false for
> consistency with the parameer listed on the CREATE SUBSCRIPTION page.
> What do you think?

Yeah, I did not change here, because other parameters were notated as
on/off. I found you started the forked thread [1] so I will revise the patch
after it was accepted.

> 
> ======
> src/backend/commands/subscriptioncmds.c
> 
> 2.3.
>   /*
> - * The changed two_phase option of the slot can't be rolled
> - * back.
> + * Altering the parameter from "true" to "false" within a
> + * transaction is prohibited. Since the apply worker does
> + * not alter the slot option to false, the backend must
> + * connect to the publisher and expressly change the
> + * parameter.
> + *
> + * There is no need to do something remarkable regarding
> + * the "false" to "true" case; the backend process alters
> + * subtwophase to LOGICALREP_TWOPHASE_STATE_PENDING once.
> + * After the subscription is enabled, a new logical
> + * replication worker requests to change the two_phase
> + * option of its slot when the initial data synchronization
> + * is done. The code path is the same as the case in which
> + * two_phase is initially set to true.
>   */
> 
> BEFORE
> ...worker requests to change the two_phase option of its slot when...
> 
> SUGGESTION
> ...worker requests to change the two_phase option of its slot from
> pending to true when...

Fixed.

> 
> ======
> src/test/subscription/t/099_twophase_added.pl
> 
> 2.4.
> +#####################
> +# Check the case that prepared transactions exist on the publisher node.
> +#
> +# Since the two_phase is "off", then normally, this PREPARE will do nothing
> +# until the COMMIT PREPARED, but in this test, we toggle the two_phase to
> +# "true" again before the COMMIT PREPARED happens.
> 
> /Since the two_phase is "off"/Since the two_phase is "false"/

Fixed.

> 
> //////////
> patch v10-0003
> //////////
> 
> ======
> src/backend/commands/subscriptioncmds.c
> 
> 3.1. AlterSubscription
> 
> + * If two_phase was enabled, there is a possibility that
> + * transactions have already been PREPARE'd. They must be
> + * checked and rolled back.
>   */
>   if (!opts.twophase)
> 
> I think it will less ambiguous if you modify this to say "If two_phase
> was previously enabled"

Fixed.

> 
> ~~~
> 
> 3.2.
> if (!opts.twophase)
> {
> List *prepared_xacts;
> 
> /*
> * Altering the parameter from "true" to "false" within
> * a transaction is prohibited. Since the apply worker
> * does not alter the slot option to false, the backend
> * must connect to the publisher and expressly change
> * the parameter.
> *
> * There is no need to do something remarkable
> * regarding the "false" to "true" case; the backend
> * process alters subtwophase to
> * LOGICALREP_TWOPHASE_STATE_PENDING once. After the
> * subscription is enabled, a new logical replication
> * worker requests to change the two_phase option of
> * its slot when the initial data synchronization is
> * done. The code path is the same as the case in which
> * two_phase is initially set to true.
> */
> if (!opts.twophase)
> PreventInTransactionBlock(isTopLevel,
> "ALTER SUBSCRIPTION ... SET (two_phase = false)");
> 
> /*
> * To prevent prepared transactions from being
> * isolated, they must manually be aborted.
> */
> if (sub->twophasestate == LOGICALREP_TWOPHASE_STATE_ENABLED &&
> (prepared_xacts = GetGidListBySubid(subid)) != NIL)
> {
> /* Abort all listed transactions */
> foreach_ptr(char, gid, prepared_xacts)
> FinishPreparedTransaction(gid, false);
> 
> list_free_deep(prepared_xacts);
> }
> }
> 
> /* Change system catalog acoordingly */
> values[Anum_pg_subscription_subtwophasestate - 1] =
> CharGetDatum(opts.twophase ?
> LOGICALREP_TWOPHASE_STATE_PENDING :
> LOGICALREP_TWOPHASE_STATE_DISABLED);
> replaces[Anum_pg_subscription_subtwophasestate - 1] = true;
> }
> 
> ~
> 
> Why is "if (!opts.twophase)" being checked at the top and then
> immediately being checed again here:
> + if (!opts.twophase)
> + PreventInTransactionBlock(isTopLevel,
> + "ALTER SUBSCRIPTION ... SET (two_phase = false)");

Oh, this was caused by wrong git operations.

> And then again here:
> CharGetDatum(opts.twophase ?
> LOGICALREP_TWOPHASE_STATE_PENDING :
> LOGICALREP_TWOPHASE_STATE_DISABLED);
> 
> There is no need to re-check a flag that was already checked, so
> clearly some of this logic/code is either wrong or redundant.

Right. I added a new variable to store the value to be changed. Thouth?

 > 
> ======
> src/test/subscription/t/099_twophase_added.pl
> 
> (Let's change these on|off to true|false to match what you did already
> in patch 0002).
> 
> 3.3.
> +#####################
> +# Check the case that prepared transactions exist on the subscriber node
> +#
> +# If the two_phase is altering from "on" to "off" and there are prepared
> +# transactions on the subscriber, they must be aborted. This test checks it.
> 
> 
> /off/false/
> 
> /on/true/

Fixed.

> 
> ~~~
> 
> 3.4.
> +# Verify the prepared transaction has been replicated to the subscriber because
> +# two_phase is set to "on".
> 
> /on/true/

Fixed.

> 
> ~~~
> 
> 3.5.
> +# Toggle the two_phase to "off" before the COMMIT PREPARED
> +$node_subscriber->safe_psql(
> +    'postgres', "
> +    ALTER SUBSCRIPTION regress_sub DISABLE;
> +    ALTER SUBSCRIPTION regress_sub SET (two_phase = off);
> +    ALTER SUBSCRIPTION regress_sub ENABLE;");
> 
> /off/false/
> 
> /two_phase = off/two_phase = false/

Fixed.

> 
> ~~~
> 
> 3.6.
> +# Verify any prepared transactions are aborted because two_phase is changed
> to
> +# "off".
> 
> /off/false/

Fixed.

> 
> //////////
> patch v10-0004
> //////////
> 
> ======
> 4.g1. GENERAL - document rendering fails
> 
> FYI - The document failed to build after I apply patch 0003. Did you try it?
> 
> In my environment it reported some unbalanced tags:
> 
> ref/create_subscription.sgml:448: parser error : Opening and ending
> tag mismatch: link line 436 and para
>          </para>
>                 ^
> ref/create_subscription.sgml:449: parser error : Opening and ending
> tag mismatch: para line 435 and listitem
>         </listitem>
> 
> etc.

Oh, I forgot to run `make check`. Sorry. It seemed that I missed to close <link> tag.

> 
> ======
> doc/src/sgml/ref/alter_subscription.sgml
> 
> 4.1.
>       <para>
>        The <literal>two_phase</literal> parameter can only be altered when
> the
> -      subscription is disabled. When altering the parameter from
> <literal>true</literal>
> -      to <literal>false</literal>, the backend process checks for any
> incomplete
> -      prepared transactions done by the logical replication worker (from when
> -      <literal>two_phase</literal> parameter was still
> <literal>true</literal>)
> -      and, if any are found, those are aborted.
> +      subscription is disabled. Altering the parameter from
> <literal>true</literal>
> +      to <literal>false</literal> will give an error when when there are
> +      prepared transactions done by the logical replication worker. If you want
> +      to alter the parameter forcibly in this case,
> +      <link
> linkend="sql-createsubscription-params-with-force-alter"><literal>force_alter
> </literal></link>
> +      option must be set to <literal>true</literal> at the same time.
>       </para>
> 
> TYPO: "when when"

Removed.

> Why is necessary to say "at the same time"?

Not needed. Fixed.

> 
> ======
> doc/src/sgml/ref/create_subscription.sgml
> 
> 4.2.
> +       <varlistentry id="sql-createsubscription-params-with-force-alter">
> +        <term><literal>force_alter</literal>
> (<type>boolean</type>)</term>
> +        <listitem>
> +         <para>
> +          Specifies if the <link
> linkend="sql-altersubscription"><command>ALTER
> SUBSCRIPTION</command>
> +          can be forced to proceed instead of giving an error. There is
> +          currently only one scenario where this parameter has any effect:
> When
> +          altering <literal>two_phase</literal> option from
> <literal>true</literal>
> +          to <literal>false</literal> it is possible for there to be incomplete
> +          prepared transactions done by the logical replication worker (from
> +          when <literal>two_phase</literal> parameter was still
> <literal>true</literal>).
> +          If <literal>force_alter</literal> is <literal>false</literal>, then
> +          this will give an error; if <literal>force_alter</literal> is
> +          <literal>true</literal>, then the incomplete prepared transactions
> +          are aborted and the alter will proceed.
> +          The default is <literal>false</literal>.
> +         </para>
> +        </listitem>
> +       </varlistentry>
> 
> IMO this will be better broken into multiple paragraphs.
> 
> 1. Specifies...
> 2. There is...
> 3. The default is...

Separated.

> 
> ======
> src/test/subscription/t/099_twophase_added.pl
> 
> (Let's change all the on|off to true|false like you already did in patch 0002.
> 
> 4.3.
> +# Try altering the two_phase option to "off." The command will fail since there
> +# is a prepared transaction and the 'force_alter' option is not specified as
> +# true.
> +my $stdout;
> +my $stderr;
> 
> /off./false/

Fixed.

[1]: https://www.postgresql.org/message-id/CAHut%2BPs-RqrggaJU5w85BbeQzw9CLmmLgADVJoJ%3Dxx_4D5CWvw%40mail.gmail.com

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