Re: Transactions involving multiple postgres foreign servers, take 2

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.

(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?

(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)

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

LSN_FORMAT_ARGS() should be used?

(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?

(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.

(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?

(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?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION