On 2020/09/07 17:59, Fujii Masao wrote:
On 2020/08/21 15:25, Masahiko Sawada wrote:
On Fri, 21 Aug 2020 at 00:36, Fujii Masao <masao.fu...@oss.nttdata.com> wrote:
On 2020/07/27 15:59, Masahiko Sawada wrote:
On Thu, 23 Jul 2020 at 22:51, Muhammad Usama <m.us...@gmail.com> wrote:
On Wed, Jul 22, 2020 at 12:42 PM Masahiko Sawada
<masahiko.saw...@2ndquadrant.com> wrote:
On Sat, 18 Jul 2020 at 01:55, Fujii Masao <masao.fu...@oss.nttdata.com> wrote:
On 2020/07/16 14:47, Masahiko Sawada wrote:
On Tue, 14 Jul 2020 at 11:19, Fujii Masao <masao.fu...@oss.nttdata.com> wrote:
On 2020/07/14 9:08, Masahiro Ikeda wrote:
I've attached the latest version patches. I've incorporated the review
comments I got so far and improved locking strategy.
Thanks for updating the patch!
+1
I'm interested in these patches and now studying them. While checking
the behaviors of the patched PostgreSQL, I got three comments.
Thank you for testing this patch!
1. We can access to the foreign table even during recovery in the HEAD.
But in the patched version, when I did that, I got the following error.
Is this intentional?
ERROR: cannot assign TransactionIds during recovery
No, it should be fixed. I'm going to fix this by not collecting
participants for atomic commit during recovery.
Thanks for trying to fix the issues!
I'd like to report one more issue. When I started new transaction
in the local server, executed INSERT in the remote server via
postgres_fdw and then quit psql, I got the following assertion failure.
TRAP: FailedAssertion("fdwxact", File: "fdwxact.c", Line: 1570)
0 postgres 0x000000010d52f3c0 ExceptionalCondition
+ 160
1 postgres 0x000000010cefbc49
ForgetAllFdwXactParticipants + 313
2 postgres 0x000000010cefff14 AtProcExit_FdwXact +
20
3 postgres 0x000000010d313fe3 shmem_exit + 179
4 postgres 0x000000010d313e7a proc_exit_prepare +
122
5 postgres 0x000000010d313da3 proc_exit + 19
6 postgres 0x000000010d35112f PostgresMain + 3711
7 postgres 0x000000010d27bb3a BackendRun + 570
8 postgres 0x000000010d27af6b BackendStartup + 475
9 postgres 0x000000010d279ed1 ServerLoop + 593
10 postgres 0x000000010d277940 PostmasterMain + 6016
11 postgres 0x000000010d1597b9 main + 761
12 libdyld.dylib 0x00007fff7161e3d5 start + 1
13 ??? 0x0000000000000003 0x0 + 3
Thank you for reporting the issue!
I've attached the latest version patch that incorporated all comments
I got so far. I've removed the patch adding the 'prefer' mode of
foreign_twophase_commit to keep the patch set simple.
I have started to review the patchset. Just a quick comment.
Patch v24-0002-Support-atomic-commit-among-multiple-foreign-ser.patch
contains changes (adding fdwxact includes) for
src/backend/executor/nodeForeignscan.c, src/backend/executor/nodeModifyTable.c
and src/backend/executor/execPartition.c files that doesn't seem to be
required with the latest version.
Thanks for your comment.
Right. I've removed these changes on the local branch.
The latest patches failed to be applied to the master branch. Could you rebase
the patches?
Thank you for letting me know. I've attached the latest version patch set.
Thanks for updating the patch!
IMO it's not easy to commit this 2PC patch at once because it's still large
and complicated. So I'm thinking it's better to separate the feature into
several parts and commit them gradually. What about separating
the feature into the following parts?
#1
Originally the server just executed xact callback that each FDW registered
when the transaction was committed. The patch changes this so that
the server manages the participants of FDW in the transaction and triggers
them to execute COMMIT or ROLLBACK. IMO this change can be applied
without 2PC feature. Thought?
Even if we commit this patch and add new interface for FDW, we would
need to keep the old interface, for the FDW providing only old interface.
#2
Originally when there was the FDW access in the transaction,
PREPARE TRANSACTION on that transaction failed with an error. The patch
allows PREPARE TRANSACTION and COMMIT/ROLLBACK PREPARED
even when FDW access occurs in the transaction. IMO this change can be
applied without *automatic* 2PC feature (i.e., PREPARE TRANSACTION and
COMMIT/ROLLBACK PREPARED are automatically executed for each FDW
inside "top" COMMIT command). Thought?
I'm not sure yet whether automatic resolution of "unresolved" prepared
transactions by the resolver process is necessary for this change or not.
If it's not necessary, it's better to exclude the resolver process from this
change, at this stage, to make the patch simpler.
#3
Finally IMO we can provide the patch supporting "automatic" 2PC for each FDW,
based on the #1 and #2 patches.
What's your opinion about this?
Also I'd like to report some typos in the patch.
+#define ServerSupportTransactionCallack(fdw_part) \
"Callack" in this macro name should be "Callback"?
+#define SeverSupportTwophaseCommit(fdw_part) \
"Sever" in this macro name should be "Server"?
+ proname => 'pg_stop_foreing_xact_resolver', provolatile => 'v', prorettype
=> 'bool',
"foreing" should be "foreign"?
+ * FdwXact entry we call get_preparedid callback to get a transaction
"get_preparedid" should be "get_prepareid"?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
