Re: Transactions involving multiple postgres foreign servers, take 2

On Sat, Jun 12, 2021 at 1:25 AM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>
>
>
> On 2021/05/11 13:37, Masahiko Sawada wrote:
> > I've attached the updated patches that incorporated comments from
> > Zhihong and Ikeda-san.
>
> Thanks for updating the patches!
>
> I'm still reading these patches, but I'd like to share some review comments
> that I found so far.

Thank you for the comments!

>
> (1)
> +/* Remove the foreign transaction from FdwXactParticipants */
> +void
> +FdwXactUnregisterXact(UserMapping *usermapping)
> +{
> + Assert(IsTransactionState());
> + RemoveFdwXactEntry(usermapping->umid);
> +}
>
> Currently there is no user of FdwXactUnregisterXact().
> This function should be removed?

I think that this function can be used by other FDW implementations
to unregister foreign transaction entry, although there is no use case
in postgres_fdw. This function corresponds to xa_unreg in the XA
specification.

>
>
> (2)
> When I ran the regression test, I got the following failure.
>
> ========= Contents of ./src/test/modules/test_fdwxact/regression.diffs
> diff -U3 /home/runner/work/postgresql/postgresql/src/test/modules/test_fdwxact/expected/test_fdwxact.out /home/runner/work/postgresql/postgresql/src/test/modules/test_fdwxact/results/test_fdwxact.out
> --- /home/runner/work/postgresql/postgresql/src/test/modules/test_fdwxact/expected/test_fdwxact.out 2021-06-10 02:19:43.808622747 +0000
> +++ /home/runner/work/postgresql/postgresql/src/test/modules/test_fdwxact/results/test_fdwxact.out 2021-06-10 02:29:53.452410462 +0000
> @@ -174,7 +174,7 @@
> SELECT count(*) FROM pg_foreign_xacts;
> count
> -------
> - 1
> + 4
> (1 row)

WIll fix.

>
>
> (3)
> + errmsg("could not read foreign transaction state from xlog at %X/%X",
> + (uint32) (lsn >> 32),
> + (uint32) lsn)));
>
> LSN_FORMAT_ARGS() should be used?

Agreed.

>
>
> (4)
> +extern void RecreateFdwXactFile(TransactionId xid, Oid umid, void *content,
> + int len);
>
> Since RecreateFdwXactFile() is used only in fdwxact.c,
> the above "extern" is not necessary?

Right.

>
>
> (5)
> +2. Pre-Commit phase (1st phase of two-phase commit)
> +we record the corresponding WAL indicating that the foreign server is involved
> +with the current transaction before doing PREPARE all foreign transactions.
> +Thus, in case we loose connectivity to the foreign server or crash ourselves,
> +we will remember that we might have prepared tranascation on the foreign
> +server, and try to resolve it when connectivity is restored or after crash
> +recovery.
>
> So currently FdwXactInsertEntry() calls XLogInsert() and XLogFlush() for
> XLOG_FDWXACT_INSERT WAL record. Additionally we should also wait there
> for WAL record to be replicated to the standby if sync replication is enabled?
> Otherwise, when the failover happens, new primary (past-standby)
> might not have enough XLOG_FDWXACT_INSERT WAL records and
> might fail to find some in-doubt foreign transactions.

But even if we wait for the record to be replicated, this problem
isn't completely resolved, right? If the server crashes before the
standy receives the record and the failover happens then the new
master doesn't have the record. I wonder if we need to have another
FDW API in order to get the list of prepared transactions from the
foreign server (FDW). For example in postgres_fdw case, it gets the
list of prepared transactions on the foreign server by executing a
query. It seems to me that this corresponds to xa_recover in the XA
specification.

> (6)
> XLogFlush() is called for each foreign transaction. So if there are many
> foreign transactions, XLogFlush() is called too frequently. Which might
> cause unnecessary performance overhead? Instead, for example,
> we should call XLogFlush() only at once in FdwXactPrepareForeignTransactions()
> after inserting all WAL records for all foreign transactions?

Agreed.

>
>
> (7)
> /* Open connection; report that we'll create a prepared statement. */
> fmstate->conn = GetConnection(user, true, &fmstate->conn_state);
> + MarkConnectionModified(user);
>
> MarkConnectionModified() should be called also when TRUNCATE on
> a foreign table is executed?

Good catch. Will fix.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/