Re: Transactions involving multiple postgres foreign servers, take 2 - Mailing list pgsql-hackers

On Wed, 19 Feb 2020 at 07:55, Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:
>
> On Tue, 11 Feb 2020 at 12:42, amul sul <sulamul@gmail.com> wrote:
> >
> > On Fri, Jan 24, 2020 at 11:31 AM Masahiko Sawada <masahiko.sawada@2ndquadrant.com> wrote:
> >>
> >> On Fri, 6 Dec 2019 at 17:33, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
> >> >
> >> > Hello.
> >> >
> >> > This is the reased (and a bit fixed) version of the patch. This
> >> > applies on the master HEAD and passes all provided tests.
> >> >
> >> > I took over this work from Sawada-san. I'll begin with reviewing the
> >> > current patch.
> >> >
> >>
> >> The previous patch set is no longer applied cleanly to the current
> >> HEAD. I've updated and slightly modified the codes.
> >>
> >> This patch set has been marked as Waiting on Author for a long time
> >> but the correct status now is Needs Review. The patch actually was
> >> updated and incorporated all review comments but they was not rebased
> >> actively.
> >>
> >> The mail[1] I posted before would be helpful to understand the current
> >> patch design and there are README in the patch and a wiki page[2].
> >>
> >> I've marked this as Needs Review.
> >>
> >
> > Hi Sawada san,
> >
> > I just had a quick look to 0001 and 0002 patch here is the few suggestions.
> >
> > patch: v27-0001:
> >
> > Typo: s/non-temprary/non-temporary
> > ----
> >
> > patch: v27-0002: (Note:The left-hand number is the line number in the v27-0002 patch):
> >
> >  138 +PostgreSQL's the global transaction manager (GTM), as a distributed transaction
> >  139 +participant The registered foreign transactions are tracked until the end of
> >
> > Full stop "." is missing after "participant"
> >
> >
> > 174 +API Contract With Transaction Management Callback Functions
> >
> > Can we just say "Transaction Management Callback Functions";
> > TOBH, I am not sure that I understand this title.
> >
> >
> >  203 +processing foreign transaction (i.g. preparing, committing or aborting) the
> >
> > Do you mean "i.e" instead of i.g. ?
> >
> >
> > 269 + * RollbackForeignTransactionAPI. Registered participant servers are identified
> >
> > Add space before between RollbackForeignTransaction and API.
> >
> >
> >  292 + * automatically so must be processed manually using by pg_resovle_fdwxact()
> >
> > Do you mean pg_resolve_foreign_xact() here?
> >
> >
> >  320 + *   the foreign transaction is authorized to update the fields from its own
> >  321 + *   one.
> >  322 +
> >  323 + * Therefore, before doing PREPARE, COMMIT PREPARED or ROLLBACK PREPARED a
> >
> > Please add asterisk '*' on line#322.
> >
> >
> >  816 +static void
> >  817 +FdwXactPrepareForeignTransactions(void)
> >  818 +{
> >  819 +   ListCell        *lcell;
> >
> > Let's have this variable name as "lc" like elsewhere.
> >
> >
> > 1036 +           ereport(ERROR, (errmsg("could not insert a foreign transaction entry"),
> > 1037 +                           errdetail("duplicate entry with transaction id %u, serverid %u, userid %u",
> > 1038 +                                  xid, serverid, userid)));
> > 1039 +   }
> >
> > Incorrect formatting.
> >
> >
> > 1166 +/*
> > 1167 + * Return true and set FdwXactAtomicCommitReady to true if the current transaction
> >
> > Do you mean ForeignTwophaseCommitIsRequired instead of FdwXactAtomicCommitReady?
> >
> >
> > 3529 +
> > 3530 +/*
> > 3531 + * FdwXactLauncherRegister
> > 3532 + *     Register a background worker running the foreign transaction
> > 3533 + *      launcher.
> > 3534 + */
> >
> > This prolog style is not consistent with the other function in the file.
> >
> >
> > And here are the few typos:
> >
> > s/conssitent/consistent
> > s/consisnts/consist
> > s/Foriegn/Foreign
> > s/tranascation/transaction
> > s/itselft/itself
> > s/rolbacked/rollbacked
> > s/trasaction/transaction
> > s/transactio/transaction
> > s/automically/automatically
> > s/CommitForeignTransaciton/CommitForeignTransaction
> > s/Similary/Similarly
> > s/FDWACT_/FDWXACT_
> > s/dink/disk
> > s/requried/required
> > s/trasactions/transactions
> > s/prepread/prepared
> > s/preapred/prepared
> > s/beging/being
> > s/gxact/xact
> > s/in-dbout/in-doubt
> > s/respecitively/respectively
> > s/transction/transaction
> > s/idenetifier/identifier
> > s/identifer/identifier
> > s/checkpoint'S/checkpoint's
> > s/fo/of
> > s/transcation/transaction
> > s/trasanction/transaction
> > s/non-temprary/non-temporary
> > s/resovler_internal.h/resolver_internal.h
> >
> >
>
> Thank you for reviewing the patch! I've incorporated all comments in
> local branch.

Attached the updated version patch sets that incorporated review
comments from Amul and Muhammad.

Regards,

--
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services