Re: Transactions involving multiple postgres foreign servers, take 2

For v32-0002-postgres_fdw-supports-commit-and-rollback-APIs.patch :

+ entry->changing_xact_state = true;
...
+ entry->changing_xact_state = abort_cleanup_failure;

I don't see return statement in between the two assignments. I wonder
why entry->changing_xact_state is set to true, and later being assigned
again.

For v32-0007-Introduce-foreign-transaction-launcher-and-resol.patch :

bq. This commits introduces to new background processes: foreign

commits introduces to new -> commit introduces two new

+FdwXactExistsXid(TransactionId xid)

Since Xid is the parameter to this method, I think the Xid suffix can be
dropped from the method name.

+ * Portions Copyright (c) 2020, PostgreSQL Global Development Group

Please correct year in the next patch set.

+FdwXactLauncherRequestToLaunch(void)

Since the launcher's job is to 'launch', I think the Launcher can be
omitted from the method name.

+/* Report shared memory space needed by FdwXactRsoverShmemInit */
+Size
+FdwXactRslvShmemSize(void)

Are both Rsover and Rslv referring to resolver ? It would be better to use
whole word which reduces confusion.
Plus, FdwXactRsoverShmemInit should be FdwXactRslvShmemInit (or
FdwXactResolveShmemInit)

+fdwxact_launch_resolver(Oid dbid)

The above method is not in camel case. It would be better if method names
are consistent (in casing).

+ errmsg("out of foreign transaction resolver slots"),
+ errhint("You might need to increase
max_foreign_transaction_resolvers.")));

It would be nice to include the value of max_foreign_xact_resolvers

For fdwxact_resolver_onexit():

+ LWLockAcquire(FdwXactLock, LW_EXCLUSIVE);
+ fdwxact->locking_backend = InvalidBackendId;
+ LWLockRelease(FdwXactLock);

There is no call to method inside the for loop which may take time. I
wonder if the lock can be obtained prior to the for loop and released
coming out of the for loop.

+FXRslvLoop(void)

Please use Resolver instead of Rslv

+ FdwXactResolveFdwXacts(held_fdwxacts, nheld);

Fdw and Xact are repeated twice each in the method name. Probably the
method name can be made shorter.

Cheers

On Thu, Jan 14, 2021 at 11:04 AM Zhihong Yu <zyu(at)yugabyte(dot)com> wrote:

> 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(dot)mshk(at)gmail(dot)com>
> wrote:
>
>> 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/
>>
>