On Sun, May 2, 2021 at 1:23 AM Zhihong Yu <z...@yugabyte.com> wrote: > > > > On Fri, Apr 30, 2021 at 9:09 PM Masahiko Sawada <sawada.m...@gmail.com> wrote: >> >> On Wed, Mar 17, 2021 at 6:03 PM Zhihong Yu <z...@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. > 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? Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
