Re: Transactions involving multiple postgres foreign servers, take 2 - Mailing list pgsql-hackers
| From | Zhihong Yu |
|---|---|
| Subject | Re: Transactions involving multiple postgres foreign servers, take 2 |
| Date | |
| Msg-id | CALNJ-vTY1rwHy+yzbKiVXJfYHw_b2yYc25nQiFc-hvaQaQGnKA@mail.gmail.com Whole thread Raw |
| In response to | Re: Transactions involving multiple postgres foreign servers, take 2 (Masahiko Sawada <sawada.mshk@gmail.com>) |
| Responses |
Re: Transactions involving multiple postgres foreign servers, take 2
Re: Transactions involving multiple postgres foreign servers, take 2 |
| List | pgsql-hackers |
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")));
+ 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.mshk@gmail.com> wrote:
On Thu, Jan 7, 2021 at 11:44 AM Zhihong Yu <zyu@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/
pgsql-hackers by date: