Re: Transactions involving multiple postgres foreign servers, take 2

On Fri, Jan 24, 2020 at 11:31 AM Masahiko Sawada <
masahiko(dot)sawada(at)2ndquadrant(dot)com> wrote:

> On Fri, 6 Dec 2019 at 17:33, Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)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

Regards,
Amul