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 Fri, Jan 15, 2021 at 4:03 AM Zhihong Yu <zyu@yugabyte.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/