Re: Transactions involving multiple postgres foreign servers, take 2

Hi,
For v32-0004-Add-PrepareForeignTransaction-API.patch :

+ * Whenever a foreign transaction is processed, the corresponding FdwXact
+ * entry is update. To avoid holding the lock during transaction
processing
+ * which may take an unpredicatable time the in-memory data of foreign

entry is update -> entry is updated

unpredictable -> unpredictable

+ int nlefts = 0;

nlefts -> nremaining

+ elog(DEBUG1, "left %u foreign transactions", nlefts);

The message can be phrased as "%u foreign transactions remaining"

+FdwXactResolveFdwXacts(int *fdwxact_idxs, int nfdwxacts)

Fdw and Xact are repeated. Seems one should suffice. How about naming the
method FdwXactResolveTransactions() ?
Similar comment for FdwXactResolveOneFdwXact(FdwXact fdwxact)

For get_fdwxact():

+ /* This entry matches the condition */
+ found = true;
+ break;

Instead of breaking and returning, you can return within the loop directly.

Cheers

On Thu, Jan 14, 2021 at 9:17 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
wrote:

> On Fri, Jan 15, 2021 at 4:03 AM Zhihong Yu <zyu(at)yugabyte(dot)com> wrote:
> >
> > Hi,
> > For v32-0008-Prepare-foreign-transactions-at-commit-time.patch :
>
> Thank you for reviewing the patch!
>
> >
> > + bool have_notwophase = false;
> >
> > Maybe name the variable have_no_twophase so that it is easier to read.
>
> Fixed.
>
> >
> > + * Two-phase commit is not required if the number of servers
> performed
> >
> > performed -> performing
>
> Fixed.
>
> >
> > + errmsg("cannot process a distributed transaction that
> has operated on a foreign server that does not support two-phase commit
> protocol"),
> > + errdetail("foreign_twophase_commit is \'required\' but
> the transaction has some foreign servers which are not capable of two-phase
> commit")));
> >
> > The lines are really long. Please wrap into more lines.
>
> Hmm, we can do that but if we do that, it makes grepping by the error
> message hard. Please refer to the documentation about the formatting
> guideline[1]:
>
> Limit line lengths so that the code is readable in an 80-column
> window. (This doesn't mean that you must never go past 80 columns. For
> instance, breaking a long error message string in arbitrary places
> just to keep the code within 80 columns is probably not a net gain in
> readability.)
>
> These changes have been made in the local branch. I'll post the
> updated patch set after incorporating all the comments.
>
> Regards,
>
> [1] https://www.postgresql.org/docs/devel/source-format.html
>
> --
> Masahiko Sawada
> EnterpriseDB: https://www.enterprisedb.com/
>