On Wed, Mar 17, 2021 at 6:03 PM Zhihong Yu <zyu@yugabyte.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.
