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

On Sat, Jul 20, 2024 at 9:31 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Thu, 18 Jul 2024 at 07:41, Hayato Kuroda (Fujitsu)
> <kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
> >
> > Dear Peter,
> >
> > Thanks for giving comments! PSA new version.
>
> Couple of suggestions:
> 1) How will user know which all transactions should be rolled back
> since the prepared transaction name will be different in subscriber
> like pg_gid_16398_750, can we mention some info on how user can
> identify these prepared transactions that should be rolled back in the
> subscriber or if this information is already available can we point it
> from here:
> + When altering <link
> linkend="sql-createsubscription-params-with-two-phase"><literal>two_phase</literal></link>
> + from <literal>true</literal> to <literal>false</literal>, the backend
> + process reports and an error if any prepared transactions done by the
> + logical replication worker (from when <literal>two_phase</literal>
> + parameter was still <literal>true</literal>) are found. You can resolve
> + prepared transactions on the publisher node, or manually roll back them
> + on the subscriber, and then try again.
>

I agree it is better to add information about this.

> 2) I'm not sure if InvalidRepOriginId is correct here, how about
> using OidIsValid in the below:
> +void
> +TwoPhaseTransactionGid(Oid subid, TransactionId xid, char *gid, int szgid)
> +{
> + Assert(subid != InvalidRepOriginId);
>

I agree with this point but please note that this patch moves this
function so that it can be used from other places. Also, I think it is
wrong to use InvalidRepOriginId as we are passing here
subscription_oid, so, ideally, we should use InvalidOid but I would
rather prefer OidIsValid() as you suggested.

--
With Regards,
Amit Kapila.