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

Dear Peter,

Thanks for reviewing! Here is new version patch.

> //////////
> Patch v9-0002
> //////////
>
> ======
> Commit Message
>
> 2.1.
> Regarding the off->on case, the logical replication already has a
> mechanism for it, so there is no need to do anything special for the
> on->off case; however, we must connect to the publisher and expressly
> change the parameter. The operation cannot be rolled back, and
> altering the parameter from "on" to "off" within a transaction is
> prohibited.
>
> In the opposite case, there is no need to prevent this because the
> logical replication worker already had the mechanism to alter the slot
> option at a convenient time.
>
> ~
>
> This explanation seems to be going around in circles, without giving
> any new information:
>
> AFAICT, "Regarding the off->on case, the logical replication already
> has a mechanism for it, so there is no need to do anything special for
> the on->off case;"
>
> is saying pretty much the same as:
>
> "In the opposite case, there is no need to prevent this because the
> logical replication worker already had the mechanism to alter the slot
> option at a convenient time."
>
> But, what I hoped for in previous review comments was an explanation
> somewhat less vague than "already has a mechanism" or "already had the
> mechanism". Can't this have just 1 or 2 lines to say WHAT is that
> existing mechanism for the "off" to "on" case, and WHY that means
> there is nothing special to do in that scenario?
>

Reworded. Thought?

> 2.2. AlterSubscription
>
> /*
> - * The changed two_phase option of the slot can't be rolled
> - * back.
> + * Since altering the two_phase option of subscriptions
> + * also leads to changing the slot option, this command
> + * cannot be rolled back. So prevent this if we are in a
> + * transaction block. In the opposite case, there is no
> + * need to prevent this because the logical replication
> + * worker already had the mechanism to alter the slot
> + * option at a convenient time.
> */
>
> (Same previous review comments, and same as my review comment for the
> commit message above).
>
> I don't think "already had the mechanism" is enough explanation.
>
> Also, the 2nd sentence doesn't make sense here because the comment
> only said "altering the slot option" -- it didn't say it was altering
> it to "on" or altering it to "off", so "the opposite case" has no
> meaning.

Fixed.

> 2.3. AlterSubscription
>
> /*
> - * Try to acquire the connection necessary for altering slot.
> + * Check the need to alter the replication slot. Failover and two_phase
> + * options are controlled by both the publisher (as a slot option) and the
> + * subscriber (as a subscription option). The slot option must be altered
> + * only when changing "on" to "off". Because in opposite case, the logical
> + * replication worker already has the mechanism to do so at a convenient
> + * time.
> + */
> + update_failover = replaces[Anum_pg_subscription_subfailover - 1];
> + update_two_phase = (replaces[Anum_pg_subscription_subtwophasestate - 1]
> &&
> + !opts.twophase);
>
> This is again the same as other review comments above. Probably, when
> some better explanation can be found for "already has the mechanism to
> do so at a convenient time." then all of these places can be changed
> using similar text.

Added a reference.

>
> //////////
> Patch v9-0003
> //////////
>
> There are some imperfect code comments but AFAIK they are the same
> ones from patch 0002. I think patch 0003 is just moving those comments
> to different places, so probably they would already be addressed by
> patch 0002.
>

The comment was moved, so no need to modify here.

> ======
> doc/src/sgml/catalogs.sgml
>
> 4.1.
> + <row>
> + <entry role="catalog_table_entry"><para role="column_definition">
> + <structfield>subforcealter</structfield> <type>bool</type>
> + </para>
> + <para>
> + If true, the subscription can be altered <literal>two_phase</literal>
> + option, even if there are prepared transactions
> + </para></entry>
> + </row>
> +
>
> BEFORE
> If true, the subscription can be altered <literal>two_phase</literal>
> option, even if there are prepared transactions
>
> SUGGESTION
> If true, then the ALTER SUBSCRIPTION command can disable
> <literal>two_phase</literal> option, even if there are uncommitted
> prepared transactions from when <literal>two_phase</literal> was
> enabled

Fixed, added a link for ALTER SUBSCRIPTION.

>
> ======
> doc/src/sgml/ref/alter_subscription.sgml
>
> 4.2.
> -
> - <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 for any incomplete
> - prepared transactions done by the logical replication worker (from when
> - <literal>two_phase</literal> parameter was still <literal>on</literal>)
> - and, if any are found, those are aborted.
> - </para>
>
> Well, I still think there ought to be some mention of the relationship
> between 'force_alter' and 'two_phase' given on this ALTER SUBSCRIPTION
> page. Then the user can cross-reference to read what the 'force_alter'
> actually does.
>

Revived the content, and added an link. Thought?

> ======
> doc/src/sgml/ref/create_subscription.sgml
>
> 4.3.
> +
> + <varlistentry id="sql-createsubscription-params-with-force-alter">
> + <term><literal>force_alter</literal>
> (<type>boolean</type>)</term>
> + <listitem>
> + <para>
> + Specifies whether the subscription can be altered
> + <literal>two_phase</literal> option, even if there are prepared
> + transactions. If specified, 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>on</literal>), if any are found, those are aborted.
> + Otherwise, Altering the parameter from <literal>on</literal> to
> + <literal>off</literal> will give an error when there are prepared
> + transactions done by the logical replication worker.
> + The default is <literal>false</literal>.
> + </para>
> + </listitem>
> + </varlistentry>
>
> This explanation seems a bit repetitive. I think it can be improved as follows:
>
> SUGGESTION
> Specifies if the ALTER SUBSCRIPTION can be forced to proceed instead
> of giving an error.
>
> There is currently only one scenario where this parameter has any
> effect: When altering two_phase option from true to false it is
> possible for there to be incomplete prepared transactions done by the
> logical replication worker (from when two_phase parameter was still
> true). If force_alter is false, then this will give an error; if
> force_alter is true, then the incomplete prepared transactions are
> aborted and the alter will proceed.
>
> The default is false.

Fixed, but added attributes.

>
> ======
> src/backend/commands/subscriptioncmds.c
>
> 4.4. CreateSubscription
>
> values[Anum_pg_subscription_subfailover - 1] = BoolGetDatum(opts.failover);
> + values[Anum_pg_subscription_subforcealter] =
> BoolGetDatum(opts.force_alter);
> values[Anum_pg_subscription_subconninfo - 1] =
>
> Hmm, looks like a bug. Shouldn't that index say -1?
>

Right, fixed.

> ~~~
> 4.5. AlterSubscription
>
> + /*
> + * Abort prepared transactions only if
> + * 'force_alter' option is true. Otherwise raise
> + * an ERROR.
> + */
> + if (IsSet(opts.specified_opts, SUBOPT_FORCE_ALTER))
> + {
> + 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, and then try again.",
> + "force_alter = true")));
> + }
> + else
> + {
> + if (!sub->forcealter)
> + 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, and then try again.",
> + "force_alter = true")));
> + }
> +
>
> IIUC this code can be simplified to remove the error duplication.
> Something like below:
>
> SUGGESTION
>
> bool raise_error = IsSet(opts.specified_opts, SUBOPT_FORCE_ALTER) ?
> !opts.force_alter : !sub->forcealter;
>
> if (raise_error)
> 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, and then try again.",
> "force_alter = true")));
>

Modified.

> ======
> src/bin/pg_dump/pg_dump.c
>
> 4.6. getSubscriptions
>
> + if (fout->remoteVersion >= 170000)
> + appendPQExpBufferStr(query,
> + " s.subforcealter\n");
> + else
> + appendPQExpBuffer(query,
> + " false AS subforcealter\n");
> +
> +
>
> 4.6a.
> Should this just be combined with the existing "if
> (fout->remoteVersion >= 170000)" for failover?

This was intentional. Features for PG17 have already been frozen, so
the patch will be pushed for PG18. After removeVersion is bumped,
I want to replace to "(fout->remoteVersion >= 180000)"

>
> ~
>
> 4.6b.
> Double blank lines.

Fixed.

> src/bin/psql/describe.c
>
> 4.7.
> + if (pset.sversion >= 170000)
> + appendPQExpBuffer(&buf,
> + ", subforcealter AS \"%s\"\n",
> + gettext_noop("Force_alter"));
>
> IMO the column title should be "Force alter" (i.e. without the underscore)
>

Fixed.

> ======
> src/include/catalog/pg_subscription.h
>
> 4.8. CATALOG
>
> + bool subforcealter; /* True if we allow to drop prepared transactions
> + * when altering two_phase "on"->"off". */
>
> I think this is not actually the description of 'force_alter'. What
> you wrote just happens to be the only case that this option currently
> works for. Maybe a more correct description is something like below.
>
> SUGGESTION
> True allows the ALTER SUBSCRIPTION command to proceed under conditions
> that would otherwise result in an error. Currently, 'force_alter' only
> has an effect when altering the two_phase option from "true" to
> "false".
>

Hmm. Seems bit long, but used yours.

> ~~~
>
> 4.9. struct Subscription
>
> + bool forcealter; /* True if we allow to drop prepared
> + * transactions when altering two_phase
> + * "on"->"off". */
>
> Ditto the previous review comment.
>

Ditto.

> ======
> src/test/regress/expected/subscription.out
>
> 4.10.
> -
> List of subscriptions
> - Name | Owner | Enabled | Publication
> | Binary | Streaming | Two-phase commit | Disable on error | Origin |
> Password required | Run as owner? | Failover | Synchronous commit |
> Conninfo | Skip LSN
> -------------------+---------------------------+---------+-------------+--------
> +-----------+------------------+------------------+--------+-------------------
> +---------------+----------+--------------------+-----------------------------+
> ----------
> - regress_testsub4 | regress_subscription_user | f | {testpub}
> | f | off | d | f | none |
> t | f | f | off |
> dbname=regress_doesnotexist | 0/0
> +
> List of
> subscriptions
> + Name | Owner | Enabled | Publication
> | Binary | Streaming | Two-phase commit | Disable on error | Origin |
> Password required | Run as owner? | Failover | Force_alter |
> Synchronous commit | Conninfo | Skip LSN
> +------------------+---------------------------+---------+-------------+-------
> -+-----------+------------------+------------------+--------+------------------
> -+---------------+----------+-------------+--------------------+---------------
> --------------+----------
>
> The column heading should be "Force alter", as already mentioned by an
> earlier review comment (#4.7)

Yeah, fixed.

> src/test/subscription/t/099_twophase_added.pl
>
> 4.11.
>
> +# Alter the two_phase with the force_alter option. Apart from the the last
> +# ALTER SUBSCRIPTION command, the command will abort the prepared
> transaction
> +# and succeed.
>
> There is typo "the the" and the wording is a bit strange. Why not just say:
>
> SUGGESTION
> Alter the two_phase true to false with the force_alter option enabled.
> This command will succeed after aborting the prepared transaction.

Fixed.

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