On Mon, May 10, 2021 at 9:38 PM Masahiko Sawada <sawada.m...@gmail.com> wrote:
> On Mon, May 3, 2021 at 11:11 PM Zhihong Yu <z...@yugabyte.com> wrote: > > > > > > > > On Mon, May 3, 2021 at 5:25 AM Masahiko Sawada <sawada.m...@gmail.com> > wrote: > >> > >> 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? > > > > > > 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
