Re: Transactions involving multiple postgres foreign servers, take 2

On Mon, May 10, 2021 at 9:38 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
wrote:

> On Mon, May 3, 2021 at 11:11 PM Zhihong Yu <zyu(at)yugabyte(dot)com> wrote:
> >
> >
> >
> > On Mon, May 3, 2021 at 5:25 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
> wrote:
> >>
> >> On Sun, May 2, 2021 at 1:23 AM Zhihong Yu <zyu(at)yugabyte(dot)com> wrote:
> >> >
> >> >
> >> >
> >> > On Fri, Apr 30, 2021 at 9:09 PM Masahiko Sawada <
> sawada(dot)mshk(at)gmail(dot)com> wrote:
> >> >>
> >> >> On Wed, Mar 17, 2021 at 6:03 PM Zhihong Yu <zyu(at)yugabyte(dot)com> wrote:
> >> >> >
> >> >> > Hi,
> >> >> > For v35-0007-Prepare-foreign-transactions-at-commit-time.patch :
> >> >>
> >> >> Thank you for reviewing the patch!
> >> >>
> >> >> >
> >> >> > With this commit, the foreign server modified within the
> transaction marked as 'modified'.
> >> >> >
> >> >> > transaction marked -> transaction is marked
> >> >>
> >> >> Will fix.
> >> >>
> >> >> >
> >> >> > +#define IsForeignTwophaseCommitRequested() \
> >> >> > + (foreign_twophase_commit > FOREIGN_TWOPHASE_COMMIT_DISABLED)
> >> >> >
> >> >> > Since the other enum is FOREIGN_TWOPHASE_COMMIT_REQUIRED, I think
> the macro should be named: IsForeignTwophaseCommitRequired.
> >> >>
> >> >> But even if foreign_twophase_commit is
> >> >> FOREIGN_TWOPHASE_COMMIT_REQUIRED, the two-phase commit is not used if
> >> >> there is only one modified server, right? It seems the name
> >> >> IsForeignTwophaseCommitRequested is fine.
> >> >>
> >> >> >
> >> >> > +static bool
> >> >> > +checkForeignTwophaseCommitRequired(bool local_modified)
> >> >> >
> >> >> > + if (!ServerSupportTwophaseCommit(fdw_part))
> >> >> > + have_no_twophase = true;
> >> >> > ...
> >> >> > + if (have_no_twophase)
> >> >> > + ereport(ERROR,
> >> >> >
> >> >> > It seems the error case should be reported within the loop. This
> way, we don't need to iterate the other participant(s).
> >> >> > Accordingly, nserverswritten should be incremented for local
> server prior to the loop. The condition in the loop would become if
> (!ServerSupportTwophaseCommit(fdw_part) && nserverswritten > 1).
> >> >> > have_no_twophase is no longer needed.
> >> >>
> >> >> Hmm, I think If we process one 2pc-non-capable server first and then
> >> >> process another one 2pc-capable server, we should raise an error but
> >> >> cannot detect that.
> >> >
> >> >
> >> > Then the check would stay as what you have in the patch:
> >> >
> >> > if (!ServerSupportTwophaseCommit(fdw_part))
> >> >
> >> > When the non-2pc-capable server is encountered, we would report the
> error in place (following the ServerSupportTwophaseCommit check) and come
> out of the loop.
> >> > have_no_twophase can be dropped.
> >>
> >> But if we processed only one non-2pc-capable server, we would raise an
> >> error but should not in that case.
> >>
> >> On second thought, I think we can track how many servers are modified
> >> or not capable of 2PC during registration and unr-egistration. Then we
> >> can consider both 2PC is required and there is non-2pc-capable server
> >> is involved without looking through all participants. Thoughts?
> >
> >
> > That is something worth trying.
> >
>
> I've attached the updated patches that incorporated comments from
> Zhihong and Ikeda-san.
>
> Regards,
>
> --
> Masahiko Sawada
> EDB: https://www.enterprisedb.com/

Hi,
For v36-0005-Prepare-foreign-transactions-at-commit-time.patch :

With this commit, the foreign server modified within the transaction
marked as 'modified'.

The verb is missing from the above sentence. 'within the transaction marked
' -> within the transaction is marked

+ /* true if modified the data on the server */

modified the data -> data is modified

+ xid = GetTopTransactionIdIfAny();
...
+ if (!TransactionIdIsValid(xid))
+ xid = GetTopTransactionId();

I wonder when the above if condition is true, would
the GetTopTransactionId() get valid xid ? It seems the two func calls are
the same.

I like the way checkForeignTwophaseCommitRequired() is structured.

Cheers