Re: Transactions involving multiple postgres foreign servers, take 2

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.

Thanks

>
> Regards,
>
> --
> Masahiko Sawada
> EDB: https://www.enterprisedb.com/
>