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

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.

Regards,

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