Re: Transactions involving multiple postgres foreign servers, take 2 - Mailing list pgsql-hackers

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/