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-vQ2c6XJVWkqgHVNb8qofjgwaeOuK_Co6VSMPHTGc9LBdQ@mail.gmail.com Whole thread Raw |
| In response to | Re: Transactions involving multiple postgres foreign servers, take 2 (Zhihong Yu <zyu@yugabyte.com>) |
| Responses |
Re: Transactions involving multiple postgres foreign servers, take 2
|
| List | pgsql-hackers |
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)
+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.")));
+ 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);
+ 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@yugabyte.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 performedperformed -> 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.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: