Re: Transactions involving multiple postgres foreign servers, take 2

On Thu, Jan 7, 2021 at 11:44 AM Zhihong Yu <zyu(at)yugabyte(dot)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/