Hi,
For v32-0008-Prepare-foreign-transactions-at-commit-time.patch :
+ bool have_notwophase = false;
Maybe name the variable have_no_twophase so that it is easier to read.
+ * Two-phase commit is not required if the number of servers performed
performed -> performing
+ 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.
On Wed, Jan 13, 2021 at 9:50 PM Masahiko Sawada <sawada.m...@gmail.com>
wrote:
> On Thu, Jan 7, 2021 at 11:44 AM Zhihong Yu <z...@yugabyte.com> wrote:
> >
> > Hi,
>
> Thank you for reviewing the patch!
>
> > For pg-foreign/v31-0004-Add-PrepareForeignTransaction-API.patch :
> >
> > However these functions are not neither committed nor aborted at
> >
> > I think the double negation was not intentional. Should be 'are neither
> ...'
>
> Fixed.
>
> >
> > For FdwXactShmemSize(), is another MAXALIGN(size) needed prior to the
> return statement ?
>
> Hmm, you mean that we need MAXALIGN(size) after adding the size of
> FdwXactData structs?
>
> Size
> FdwXactShmemSize(void)
> {
> Size size;
>
> /* Size for foreign transaction information array */
> size = offsetof(FdwXactCtlData, fdwxacts);
> size = add_size(size, mul_size(max_prepared_foreign_xacts,
> sizeof(FdwXact)));
> size = MAXALIGN(size);
> size = add_size(size, mul_size(max_prepared_foreign_xacts,
> sizeof(FdwXactData)));
>
> return size;
> }
>
> I don't think we need to do that. Looking at other similar code such
> as TwoPhaseShmemSize() doesn't do that. Why do you think we need that?
>
> >
> > + fdwxact = FdwXactInsertFdwXactEntry(xid, fdw_part);
> >
> > For the function name, Fdw and Xact appear twice, each. Maybe one of
> them can be dropped ?
>
> Agreed. Changed to FdwXactInsertEntry().
>
> >
> > + * we don't need to anything for this participant because all
> foreign
> >
> > 'need to' -> 'need to do'
>
> Fixed.
>
> >
> > + else if (TransactionIdDidAbort(xid))
> > + return FDWXACT_STATUS_ABORTING;
> > +
> > the 'else' can be omitted since the preceding if would return.
>
> Fixed.
>
> >
> > + if (max_prepared_foreign_xacts <= 0)
> >
> > I wonder when the value for max_prepared_foreign_xacts would be negative
> (and whether that should be considered an error).
> >
>
> Fixed to (max_prepared_foreign_xacts == 0)
>
> Attached the updated version patch set.
>
> Regards,
>
> --
> Masahiko Sawada
> EnterpriseDB: https://www.enterprisedb.com/
>
