Re: [HACKERS] Transactions involving multiple postgres foreign servers, take 2

On Mon, Jun 11, 2018 at 1:53 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Jun 5, 2018 at 7:13 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Sat, May 26, 2018 at 12:25 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Fri, May 18, 2018 at 11:21 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Regarding to API design, should we use 2PC for a distributed
transaction if both two or more 2PC-capable foreign servers and
2PC-non-capable foreign server are involved with it? Or should we end
up with an error? the 2PC-non-capable server might be either that has
2PC functionality but just disables it or that doesn't have it.

It seems to me that this is functionality that many people will not
want to use. First, doing a PREPARE and then a COMMIT for each FDW
write transaction is bound to be more expensive than just doing a
COMMIT. Second, because the default value of
max_prepared_transactions is 0, this can only work at all if special
configuration has been done on the remote side. Because of the second
point in particular, it seems to me that the default for this new
feature must be "off". It would make to ship a default configuration
of PostgreSQL that doesn't work with the default configuration of
postgres_fdw, and I do not think we want to change the default value
of max_prepared_transactions. It was changed from 5 to 0 a number of
years back for good reason.

I'm not sure that many people will not want to use this feature
because it seems to me that there are many people who don't want to
use the database that is missing transaction atomicity. But I agree
that this feature should not be enabled by default as we disable 2PC
by default.

So, I think the question could be broadened a bit: how you enable this
feature if you want it, and what happens if you want it but it's not
available for your choice of FDW? One possible enabling method is a
GUC (e.g. foreign_twophase_commit). It could be true/false, with true
meaning use PREPARE for all FDW writes and fail if that's not
supported, or it could be three-valued, like require/prefer/disable,
with require throwing an error if PREPARE support is not available and
prefer using PREPARE where available but without failing when it isn't
available. Another possibility could be to make it an FDW option,
possibly capable of being set at multiple levels (e.g. server or
foreign table). If any FDW involved in the transaction demands
distributed 2PC semantics then the whole transaction must have those
semantics or it fails. I was previous leaning toward the latter
approach, but I guess now the former approach is sounding better. I'm
not totally certain I know what's best here.

I agree that the former is better. That way, we also can control that
parameter at transaction level. If we allow the 'prefer' behavior we
need to manage not only 2PC-capable foreign server but also
2PC-non-capable foreign server. It requires all FDW to call the
registration function. So I think two-values parameter would be
better.

BTW, sorry for late submitting the updated patch. I'll post the
updated patch in this week but I'd like to share the new APIs design
beforehand.

Attached updated patches.

I've changed the new APIs to 5 functions and 1 registration function
because the rollback API can be called by both backend process and
resolver process which is not good design. The latest version patches
incorporated all comments I got except for documentation about overall
point to user. I'm considering what contents I should document it
there. I'll write it during the code patch is getting reviewed. The
basic design of new patches is almost same as the previous mail I
sent.

I introduced 5 new FDW APIs: PrepareForeignTransaction,
CommitForeignTransaction, RollbackForeignTransaction,
ResolveForeignTransaction and IsTwophaseCommitEnabled.
ResolveForeignTransaction is normally called by resolver process
whereas other four functions are called by backend process. Also I
introduced a registration function FdwXactRegisterForeignTransaction.
FDW that wish to support atomic commit requires to call this function
when a transaction opens on the foreign server. Registered foreign
transactions are controlled by the foreign transaction manager of
Postgres core and calls APIs at appropriate timing. It means that the
foreign transaction manager controls only foreign servers that are
capable of 2PC. For 2PC-non-capable foreign server, FDW must use
XactCallback to control the foreign transaction. 2PC is used at commit
when the distributed transaction modified data on two or more servers
including local server and user requested by foreign_twophase_commit
GUC parameter. All foreign transactions are prepared during pre-commit
and then commit locally. After committed locally wait for resolver
process to resolve all prepared foreign transactions. The waiting
backend is released (that is, returns the prompt to client) either
when all foreign transactions are resolved or when user requested to
waiting. If 2PC is not required, a foreign transaction is committed
during pre-commit phase of local transaction. IsTwophaseCommitEnabled
is called whenever the transaction begins to modify data on foreign
server. This is required to track whether the transaction modified
data on the foreign server that doesn't support or enable 2PC.

Atomic commit among multiple foreign servers is crash-safe. If the
coordinator server crashes during atomic commit, the foreign
transaction participants and their status are recovered during WAL
apply. Recovered foreign transactions are in doubt-state, aka dangling
transactions. If database has such transactions resolver process
periodically tries to resolve them.

I'll register this patch to next CF. Feedback is very welcome.

I attached the updated version patch as the previous versions conflict
with the current HEAD.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Fri, Aug 03, 2018 at 05:52:24PM +0900, Masahiko Sawada wrote:

I attached the updated version patch as the previous versions conflict
with the current HEAD.

Please note that the latest patch set does not apply anymore, so this
patch is moved to next CF, waiting on author.
--
Michael

On Tue, Oct 2, 2018 at 3:10 PM Michael Paquier <michael@paquier.xyz> wrote:

On Fri, Aug 03, 2018 at 05:52:24PM +0900, Masahiko Sawada wrote:

I attached the updated version patch as the previous versions conflict
with the current HEAD.

Please note that the latest patch set does not apply anymore, so this
patch is moved to next CF, waiting on author.

Thank you! Attached the latest version patches.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

The following review has been posted through the commitfest application:
make installcheck-world: tested, failed
Implements feature: not tested
Spec compliant: not tested
Documentation: tested, failed

I am hoping I am not out of order in writing this before the commitfest starts. The patch is big and long and so wanted to start on this while traffic is slow.

I find this patch quite welcome and very close to a minimum viable version. The few significant limitations can be resolved later. One thing I may have missed in the documentation is a discussion of the limits of the current approach. I think this would be important to document because the caveats of the current approach are significant, but the people who need it will have the knowledge to work with issues if they come up.

The major caveat I see in our past discussions and (if I read the patch correctly) is that the resolver goes through global transactions sequentially and does not move on to the next until the previous one is resolved. This means that if I have a global transaction on server A, with foreign servers B and C, and I have another one on server A with foreign servers C and D, if server B goes down at the wrong moment, the background worker does not look like it will detect the failure and move on to try to resolve the second, so server D will have a badly set vacuum horizon until this is resolved. Also if I read the patch correctly, it looks like one can invoke SQL commands to remove the bad transaction to allow processing to continue and manual resolution (this is good and necessary because in this area there is no ability to have perfect recoverability without occasional administrative action). I would really like to see more documentation of failure cases and appropriate administrative action at present. Otherwise this is I think a minimum viable addition and I think we want it.

It is possible i missed that in the documentation. If so, my objection stands aside. If it is welcome I am happy to take a first crack at such docs.

To my mind thats the only blocker in the code (but see below). I can say without a doubt that I would expect we would use this feature once available.

------------------

Testing however failed.

make installcheck-world fails with errors like the following:

 -- Modify foreign server and raise an error
  BEGIN;
  INSERT INTO ft7_twophase VALUES(8);
+ ERROR:  prepread foreign transactions are disabled
+ HINT:  Set max_prepared_foreign_transactions to a nonzero value.
  INSERT INTO ft8_twophase VALUES(NULL); -- violation
! ERROR:  current transaction is aborted, commands ignored until end of transaction block
  ROLLBACK;
  SELECT * FROM ft7_twophase;
! ERROR:  prepread foreign transactions are disabled
! HINT:  Set max_prepared_foreign_transactions to a nonzero value.
  SELECT * FROM ft8_twophase;
! ERROR:  prepread foreign transactions are disabled
! HINT:  Set max_prepared_foreign_transactions to a nonzero value.
  -- Rollback foreign transaction that involves both 2PC-capable
  -- and 2PC-non-capable foreign servers.
  BEGIN;
  INSERT INTO ft8_twophase VALUES(7);
+ ERROR:  prepread foreign transactions are disabled
+ HINT:  Set max_prepared_foreign_transactions to a nonzero value.
  INSERT INTO ft9_not_twophase VALUES(7);
+ ERROR:  current transaction is aborted, commands ignored until end of transaction block
  ROLLBACK;
  SELECT * FROM ft8_twophase;
! ERROR:  prepread foreign transactions are disabled
! HINT:  Set max_prepared_foreign_transactions to a nonzero value.

make installcheck in the contrib directory shows the same, so that's the easiest way of reproducing, at least on a new installation. I think the test cases will have to handle that sort of setup.

make check in the contrib directory passes.

For reasons of test failures, I am setting this back to waiting on author.

------------------
I had a few other thoughts that I figure are worth sharing with the community on this patch with the idea that once it is in place, this may open up more options for collaboration in the area of federated and distributed storage generally. I could imagine other foreign data wrappers using this API, and folks might want to refactor out the atomic handling part so that extensions that do not use the foreign data wrapper structure could use it as well (while this looks like a classic SQL/MED issue, I am not sure that only foreign data wrappers would be interested in the API.

The new status of this patch is: Waiting on Author

On Wed, Oct 3, 2018 at 9:41 AM Chris Travers <chris.travers@gmail.com>
wrote:

The following review has been posted through the commitfest application:
make installcheck-world: tested, failed
Implements feature: not tested
Spec compliant: not tested
Documentation: tested, failed

Also one really minor point: I think this is a typo (maX vs max)?

(errmsg("preparing foreign transactions (max_prepared_foreign_transactions

0) requires maX_foreign_xact_resolvers > 0")));

--
Best Regards,
Chris Travers
Head of Database

Tel: +49 162 9037 210 | Skype: einhverfr | www.adjust.com
Saarbrücker Straße 37a, 10405 Berlin

On Wed, Oct 3, 2018 at 9:56 AM Chris Travers <chris.travers@adjust.com>
wrote:

On Wed, Oct 3, 2018 at 9:41 AM Chris Travers <chris.travers@gmail.com>
wrote:

(errmsg("preparing foreign transactions

(max_prepared_foreign_transactions > 0) requires maX_foreign_xact_resolvers

0")));

Two more critical notes here which I think are small blockers.

The error message above references a config variable that does not exist.

The correct name of the config parameter is
max_foreign_transaction_resolvers

Setting that along with the following to 10 caused the tests to pass, but
again it fails on default configs:

max_prepared_foreign_transactions, max_prepared_transactions

--
Best Regards,
Chris Travers
Head of Database

Tel: +49 162 9037 210 | Skype: einhverfr | www.adjust.com
Saarbrücker Straße 37a, 10405 Berlin

--
Best Regards,
Chris Travers
Head of Database

Tel: +49 162 9037 210 | Skype: einhverfr | www.adjust.com
Saarbrücker Straße 37a, 10405 Berlin

On Wed, Oct 3, 2018 at 9:41 AM Chris Travers <chris.travers@gmail.com>
wrote:

The following review has been posted through the commitfest application:
make installcheck-world: tested, failed
Implements feature: not tested
Spec compliant: not tested
Documentation: tested, failed

I am hoping I am not out of order in writing this before the commitfest
starts. The patch is big and long and so wanted to start on this while
traffic is slow.

I find this patch quite welcome and very close to a minimum viable
version. The few significant limitations can be resolved later. One thing
I may have missed in the documentation is a discussion of the limits of the
current approach. I think this would be important to document because the
caveats of the current approach are significant, but the people who need it
will have the knowledge to work with issues if they come up.

The major caveat I see in our past discussions and (if I read the patch
correctly) is that the resolver goes through global transactions
sequentially and does not move on to the next until the previous one is
resolved. This means that if I have a global transaction on server A, with
foreign servers B and C, and I have another one on server A with foreign
servers C and D, if server B goes down at the wrong moment, the background
worker does not look like it will detect the failure and move on to try to
resolve the second, so server D will have a badly set vacuum horizon until
this is resolved. Also if I read the patch correctly, it looks like one
can invoke SQL commands to remove the bad transaction to allow processing
to continue and manual resolution (this is good and necessary because in
this area there is no ability to have perfect recoverability without
occasional administrative action). I would really like to see more
documentation of failure cases and appropriate administrative action at
present. Otherwise this is I think a minimum viable addition and I think
we want it.

It is possible i missed that in the documentation. If so, my objection
stands aside. If it is welcome I am happy to take a first crack at such
docs.

After further testing I am pretty sure I misread the patch. It looks like
one can have multiple resolvers which can, in fact, work through a queue
together solving this problem. So the objection above is not valid and I
withdraw that objection. I will re-review the docs in light of the
experience.

To my mind thats the only blocker in the code (but see below). I can say
without a doubt that I would expect we would use this feature once
available.

------------------

Testing however failed.

make installcheck-world fails with errors like the following:

-- Modify foreign server and raise an error
BEGIN;
INSERT INTO ft7_twophase VALUES(8);
+ ERROR:  prepread foreign transactions are disabled
+ HINT:  Set max_prepared_foreign_transactions to a nonzero value.
INSERT INTO ft8_twophase VALUES(NULL); -- violation
! ERROR:  current transaction is aborted, commands ignored until end of
transaction block
ROLLBACK;
SELECT * FROM ft7_twophase;
! ERROR:  prepread foreign transactions are disabled
! HINT:  Set max_prepared_foreign_transactions to a nonzero value.
SELECT * FROM ft8_twophase;
! ERROR:  prepread foreign transactions are disabled
! HINT:  Set max_prepared_foreign_transactions to a nonzero value.
-- Rollback foreign transaction that involves both 2PC-capable
-- and 2PC-non-capable foreign servers.
BEGIN;
INSERT INTO ft8_twophase VALUES(7);
+ ERROR:  prepread foreign transactions are disabled
+ HINT:  Set max_prepared_foreign_transactions to a nonzero value.
INSERT INTO ft9_not_twophase VALUES(7);
+ ERROR:  current transaction is aborted, commands ignored until end of
transaction block
ROLLBACK;
SELECT * FROM ft8_twophase;
! ERROR:  prepread foreign transactions are disabled
! HINT:  Set max_prepared_foreign_transactions to a nonzero value.

make installcheck in the contrib directory shows the same, so that's the
easiest way of reproducing, at least on a new installation. I think the
test cases will have to handle that sort of setup.

make check in the contrib directory passes.

For reasons of test failures, I am setting this back to waiting on author.

------------------
I had a few other thoughts that I figure are worth sharing with the
community on this patch with the idea that once it is in place, this may
open up more options for collaboration in the area of federated and
distributed storage generally. I could imagine other foreign data wrappers
using this API, and folks might want to refactor out the atomic handling
part so that extensions that do not use the foreign data wrapper structure
could use it as well (while this looks like a classic SQL/MED issue, I am
not sure that only foreign data wrappers would be interested in the API.

The new status of this patch is: Waiting on Author

--
Best Regards,
Chris Travers
Head of Database

Tel: +49 162 9037 210 | Skype: einhverfr | www.adjust.com
Saarbrücker Straße 37a, 10405 Berlin

On Wed, Oct 3, 2018 at 6:02 PM Chris Travers <chris.travers@adjust.com> wrote:

On Wed, Oct 3, 2018 at 9:41 AM Chris Travers <chris.travers@gmail.com> wrote:

The following review has been posted through the commitfest application:
make installcheck-world: tested, failed
Implements feature: not tested
Spec compliant: not tested
Documentation: tested, failed

I am hoping I am not out of order in writing this before the commitfest starts. The patch is big and long and so wanted to start on this while traffic is slow.

I find this patch quite welcome and very close to a minimum viable version. The few significant limitations can be resolved later. One thing I may have missed in the documentation is a discussion of the limits of the current approach. I think this would be important to document because the caveats of the current approach are significant, but the people who need it will have the knowledge to work with issues if they come up.

The major caveat I see in our past discussions and (if I read the patch correctly) is that the resolver goes through global transactions sequentially and does not move on to the next until the previous one is resolved. This means that if I have a global transaction on server A, with foreign servers B and C, and I have another one on server A with foreign servers C and D, if server B goes down at the wrong moment, the background worker does not look like it will detect the failure and move on to try to resolve the second, so server D will have a badly set vacuum horizon until this is resolved. Also if I read the patch correctly, it looks like one can invoke SQL commands to remove the bad transaction to allow processing to continue and manual resolution (this is good and necessary because in this area there is no ability to have perfect recoverability without occasional administrative action). I would really like to see more documentation of failure cases and appropriate administrative action at present. Otherwise this is I think a minimum viable addition and I think we want it.

It is possible i missed that in the documentation. If so, my objection stands aside. If it is welcome I am happy to take a first crack at such docs.

Thank you for reviewing the patch!

After further testing I am pretty sure I misread the patch. It looks like one can have multiple resolvers which can, in fact, work through a queue together solving this problem. So the objection above is not valid and I withdraw that objection. I will re-review the docs in light of the experience.

Actually the patch doesn't solve this problem; the foreign transaction
resolver processes distributed transactions sequentially. But since
one resolver process is responsible for one database the backend
connecting to another database can complete the distributed
transaction. I understood the your concern and agreed to solve this
problem. I'll address it in the next patch.

To my mind thats the only blocker in the code (but see below). I can say without a doubt that I would expect we would use this feature once available.

------------------

Testing however failed.

make installcheck-world fails with errors like the following:

-- Modify foreign server and raise an error
BEGIN;
INSERT INTO ft7_twophase VALUES(8);
+ ERROR:  prepread foreign transactions are disabled
+ HINT:  Set max_prepared_foreign_transactions to a nonzero value.
INSERT INTO ft8_twophase VALUES(NULL); -- violation
! ERROR:  current transaction is aborted, commands ignored until end of transaction block
ROLLBACK;
SELECT * FROM ft7_twophase;
! ERROR:  prepread foreign transactions are disabled
! HINT:  Set max_prepared_foreign_transactions to a nonzero value.
SELECT * FROM ft8_twophase;
! ERROR:  prepread foreign transactions are disabled
! HINT:  Set max_prepared_foreign_transactions to a nonzero value.
-- Rollback foreign transaction that involves both 2PC-capable
-- and 2PC-non-capable foreign servers.
BEGIN;
INSERT INTO ft8_twophase VALUES(7);
+ ERROR:  prepread foreign transactions are disabled
+ HINT:  Set max_prepared_foreign_transactions to a nonzero value.
INSERT INTO ft9_not_twophase VALUES(7);
+ ERROR:  current transaction is aborted, commands ignored until end of transaction block
ROLLBACK;
SELECT * FROM ft8_twophase;
! ERROR:  prepread foreign transactions are disabled
! HINT:  Set max_prepared_foreign_transactions to a nonzero value.

make installcheck in the contrib directory shows the same, so that's the easiest way of reproducing, at least on a new installation. I think the test cases will have to handle that sort of setup.

The 'make installcheck' is a regression test mode to do the tests to
the existing installation. If the installation disables atomic commit
feature (e.g. max_prepared_foreign_transaction etc) the test will fail
because the feature is disabled by default.

make check in the contrib directory passes.

For reasons of test failures, I am setting this back to waiting on author.

------------------
I had a few other thoughts that I figure are worth sharing with the community on this patch with the idea that once it is in place, this may open up more options for collaboration in the area of federated and distributed storage generally. I could imagine other foreign data wrappers using this API, and folks might want to refactor out the atomic handling part so that extensions that do not use the foreign data wrapper structure could use it as well (while this looks like a classic SQL/MED issue, I am not sure that only foreign data wrappers would be interested in the API.

The new status of this patch is: Waiting on Author

Also, I'll update the doc in the next patch that I'll post on this week.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Wed, Oct 10, 2018 at 1:34 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Wed, Oct 3, 2018 at 6:02 PM Chris Travers <chris.travers@adjust.com> wrote:

On Wed, Oct 3, 2018 at 9:41 AM Chris Travers <chris.travers@gmail.com> wrote:

The following review has been posted through the commitfest application:
make installcheck-world: tested, failed
Implements feature: not tested
Spec compliant: not tested
Documentation: tested, failed

I am hoping I am not out of order in writing this before the commitfest starts. The patch is big and long and so wanted to start on this while traffic is slow.

I find this patch quite welcome and very close to a minimum viable version. The few significant limitations can be resolved later. One thing I may have missed in the documentation is a discussion of the limits of the current approach. I think this would be important to document because the caveats of the current approach are significant, but the people who need it will have the knowledge to work with issues if they come up.

The major caveat I see in our past discussions and (if I read the patch correctly) is that the resolver goes through global transactions sequentially and does not move on to the next until the previous one is resolved. This means that if I have a global transaction on server A, with foreign servers B and C, and I have another one on server A with foreign servers C and D, if server B goes down at the wrong moment, the background worker does not look like it will detect the failure and move on to try to resolve the second, so server D will have a badly set vacuum horizon until this is resolved. Also if I read the patch correctly, it looks like one can invoke SQL commands to remove the bad transaction to allow processing to continue and manual resolution (this is good and necessary because in this area there is no ability to have perfect recoverability without occasional administrative action). I would really like to see more documentation of failure cases and appropriate administrative action at present. Otherwise this is I think a minimum viable addition and I think we want it.

It is possible i missed that in the documentation. If so, my objection stands aside. If it is welcome I am happy to take a first crack at such docs.

Thank you for reviewing the patch!

After further testing I am pretty sure I misread the patch. It looks like one can have multiple resolvers which can, in fact, work through a queue together solving this problem. So the objection above is not valid and I withdraw that objection. I will re-review the docs in light of the experience.

Actually the patch doesn't solve this problem; the foreign transaction
resolver processes distributed transactions sequentially. But since
one resolver process is responsible for one database the backend
connecting to another database can complete the distributed
transaction. I understood the your concern and agreed to solve this
problem. I'll address it in the next patch.

To my mind thats the only blocker in the code (but see below). I can say without a doubt that I would expect we would use this feature once available.

------------------

Testing however failed.

make installcheck-world fails with errors like the following:

-- Modify foreign server and raise an error
BEGIN;
INSERT INTO ft7_twophase VALUES(8);
+ ERROR:  prepread foreign transactions are disabled
+ HINT:  Set max_prepared_foreign_transactions to a nonzero value.
INSERT INTO ft8_twophase VALUES(NULL); -- violation
! ERROR:  current transaction is aborted, commands ignored until end of transaction block
ROLLBACK;
SELECT * FROM ft7_twophase;
! ERROR:  prepread foreign transactions are disabled
! HINT:  Set max_prepared_foreign_transactions to a nonzero value.
SELECT * FROM ft8_twophase;
! ERROR:  prepread foreign transactions are disabled
! HINT:  Set max_prepared_foreign_transactions to a nonzero value.
-- Rollback foreign transaction that involves both 2PC-capable
-- and 2PC-non-capable foreign servers.
BEGIN;
INSERT INTO ft8_twophase VALUES(7);
+ ERROR:  prepread foreign transactions are disabled
+ HINT:  Set max_prepared_foreign_transactions to a nonzero value.
INSERT INTO ft9_not_twophase VALUES(7);
+ ERROR:  current transaction is aborted, commands ignored until end of transaction block
ROLLBACK;
SELECT * FROM ft8_twophase;
! ERROR:  prepread foreign transactions are disabled
! HINT:  Set max_prepared_foreign_transactions to a nonzero value.

make installcheck in the contrib directory shows the same, so that's the easiest way of reproducing, at least on a new installation. I think the test cases will have to handle that sort of setup.

The 'make installcheck' is a regression test mode to do the tests to
the existing installation. If the installation disables atomic commit
feature (e.g. max_prepared_foreign_transaction etc) the test will fail
because the feature is disabled by default.

make check in the contrib directory passes.

For reasons of test failures, I am setting this back to waiting on author.

------------------
I had a few other thoughts that I figure are worth sharing with the community on this patch with the idea that once it is in place, this may open up more options for collaboration in the area of federated and distributed storage generally. I could imagine other foreign data wrappers using this API, and folks might want to refactor out the atomic handling part so that extensions that do not use the foreign data wrapper structure could use it as well (while this looks like a classic SQL/MED issue, I am not sure that only foreign data wrappers would be interested in the API.

The new status of this patch is: Waiting on Author

Also, I'll update the doc in the next patch that I'll post on this week.

Attached the updated version of patches. What I changed from the
previous version are,

* Enabled processing subsequent distributed transactions even when
previous distributed transaction continues to fail due to participants
error.
To implement this, I've splited the waiting queue into two queues: the
active queue and retry queue. All backend inserts itself to the active
queue firstly and change its state to FDW_XACT_WAITING. Once the
resolver process failed to resolve the distributed transaction, it
move the backend entry in the active queue to the retry queue and
change its state to FDW_XACT_WAITING_RETRY. The backend entries in the
active queue are processed each commit time whereas entries in the
retry queue are processed at interval of
foreign_transaction_resolution_retry_interval.

* Updated docs, added the new section "Distributed Transaction" at
Chapter 33 to explain the concept to users

* Moved atomic commit codes into src/backend/access/fdwxact directory.

* Some bug fixes.

Please reivew them.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Hello.

# It took a long time to come here..

At Fri, 19 Oct 2018 21:38:35 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in <CAD21AoCBf-AJup-_ARfpqR42gJQ_XjNsvv-XE0rCOCLEkT=HCg@mail.gmail.com>

On Wed, Oct 10, 2018 at 1:34 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

...

* Updated docs, added the new section "Distributed Transaction" at
Chapter 33 to explain the concept to users

* Moved atomic commit codes into src/backend/access/fdwxact directory.

* Some bug fixes.

Please reivew them.

I have some comments, with apologize in advance for possible
duplicate or conflict with others' comments so far.

0001:

This sets XACT_FLAG_WROTENONTEMPREL when RELPERSISTENT_PERMANENT
relation is modified. Isn't it needed when UNLOGGED tables are
modified? It may be better that we have dedicated classification
macro or function.

The flag is handled in heapam.c. I suppose that it should be done
in the upper layer considering coming pluggable storage.
(X_F_ACCESSEDTEMPREL is set in heapam, but..)

0002:

The name FdwXactParticipantsForAC doesn't sound good for me. How
about FdwXactAtomicCommitPartitcipants?

Well, as the file comment of fdwxact.c,
FdwXactRegisterTransaction is called from FDW driver and
F_X_MarkForeignTransactionModified is called from executor. I
think that we should clarify who is responsible to the whole
sequence. Since the state of local tables affects, I suppose
executor is that. Couldn't we do the whole thing within executor
side? I'm not sure but I feel that
F_X_RegisterForeignTransaction can be a part of
F_X_MarkForeignTransactionModified. The callers of
MarkForeignTransactionModified can find whether the table is
involved in 2pc by IsTwoPhaseCommitEnabled interface.

if (foreign_twophase_commit == true &&
((MyXactFlags & XACT_FLAGS_FDWNOPREPARE) != 0) )
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot COMMIT a distributed transaction that has operated on foreign server that doesn't support atomic commit")));

The error is emitted when a the GUC is turned off in the
trasaction where MarkTransactionModify'ed. I think that the
number of the variables' possible states should be reduced for
simplicity. For example in the case, once foreign_twopase_commit
is checked in a transaction, subsequent changes in the
transaction should be ignored during the transaction.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Oct 23, 2018 at 12:54 PM Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:

Hello.

# It took a long time to come here..

At Fri, 19 Oct 2018 21:38:35 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in <CAD21AoCBf-AJup-_ARfpqR42gJQ_XjNsvv-XE0rCOCLEkT=HCg@mail.gmail.com>

On Wed, Oct 10, 2018 at 1:34 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

...

* Updated docs, added the new section "Distributed Transaction" at
Chapter 33 to explain the concept to users

* Moved atomic commit codes into src/backend/access/fdwxact directory.

* Some bug fixes.

Please reivew them.

I have some comments, with apologize in advance for possible
duplicate or conflict with others' comments so far.

Thank youf so much for reviewing this patch!

0001:

This sets XACT_FLAG_WROTENONTEMPREL when RELPERSISTENT_PERMANENT
relation is modified. Isn't it needed when UNLOGGED tables are
modified? It may be better that we have dedicated classification
macro or function.

I think even if we do atomic commit for modifying the an UNLOGGED
table and a remote table the data will get inconsistent if the local
server crashes. For example, if the local server crashes after
prepared the transaction on foreign server but before the local commit
and, we will lose the all data of the local UNLOGGED table whereas the
modification of remote table is rollbacked. In case of persistent
tables, the data consistency is left. So I think the keeping data
consistency between remote data and local unlogged table is difficult
and want to leave it as a restriction for now. Am I missing something?

The flag is handled in heapam.c. I suppose that it should be done
in the upper layer considering coming pluggable storage.
(X_F_ACCESSEDTEMPREL is set in heapam, but..)

Yeah, or we can set the flag after heap_insert in ExecInsert.

0002:

The name FdwXactParticipantsForAC doesn't sound good for me. How
about FdwXactAtomicCommitPartitcipants?

+1, will fix it.

Well, as the file comment of fdwxact.c,
FdwXactRegisterTransaction is called from FDW driver and
F_X_MarkForeignTransactionModified is called from executor. I
think that we should clarify who is responsible to the whole
sequence. Since the state of local tables affects, I suppose
executor is that. Couldn't we do the whole thing within executor
side? I'm not sure but I feel that
F_X_RegisterForeignTransaction can be a part of
F_X_MarkForeignTransactionModified. The callers of
MarkForeignTransactionModified can find whether the table is
involved in 2pc by IsTwoPhaseCommitEnabled interface.

Indeed. We can register foreign servers by executor while FDWs don't
need to register anything. I will remove the registration function so
that FDW developers don't need to call the register function but only
need to provide atomic commit APIs.

if (foreign_twophase_commit == true &&
((MyXactFlags & XACT_FLAGS_FDWNOPREPARE) != 0) )
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot COMMIT a distributed transaction that has operated on foreign server that doesn't support atomic commit")));

The error is emitted when a the GUC is turned off in the
trasaction where MarkTransactionModify'ed. I think that the
number of the variables' possible states should be reduced for
simplicity. For example in the case, once foreign_twopase_commit
is checked in a transaction, subsequent changes in the
transaction should be ignored during the transaction.

I might have not gotten your comment correctly but since the
foreign_twophase_commit is a PGC_USERSET parameter I think we need to
check it at commit time. Also we need to keep participant servers even
when foreign_twophase_commit is off if both max_prepared_foreign_xacts
and max_foreign_xact_resolvers are > 0.

I will post the updated patch in this week.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Wed, Oct 24, 2018 at 9:06 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Oct 23, 2018 at 12:54 PM Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:

Hello.

# It took a long time to come here..

At Fri, 19 Oct 2018 21:38:35 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in <CAD21AoCBf-AJup-_ARfpqR42gJQ_XjNsvv-XE0rCOCLEkT=HCg@mail.gmail.com>

On Wed, Oct 10, 2018 at 1:34 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

...

* Updated docs, added the new section "Distributed Transaction" at
Chapter 33 to explain the concept to users

* Moved atomic commit codes into src/backend/access/fdwxact directory.

* Some bug fixes.

Please reivew them.

I have some comments, with apologize in advance for possible
duplicate or conflict with others' comments so far.

Thank youf so much for reviewing this patch!

0001:

This sets XACT_FLAG_WROTENONTEMPREL when RELPERSISTENT_PERMANENT
relation is modified. Isn't it needed when UNLOGGED tables are
modified? It may be better that we have dedicated classification
macro or function.

I think even if we do atomic commit for modifying the an UNLOGGED
table and a remote table the data will get inconsistent if the local
server crashes. For example, if the local server crashes after
prepared the transaction on foreign server but before the local commit
and, we will lose the all data of the local UNLOGGED table whereas the
modification of remote table is rollbacked. In case of persistent
tables, the data consistency is left. So I think the keeping data
consistency between remote data and local unlogged table is difficult
and want to leave it as a restriction for now. Am I missing something?

The flag is handled in heapam.c. I suppose that it should be done
in the upper layer considering coming pluggable storage.
(X_F_ACCESSEDTEMPREL is set in heapam, but..)

Yeah, or we can set the flag after heap_insert in ExecInsert.

0002:

The name FdwXactParticipantsForAC doesn't sound good for me. How
about FdwXactAtomicCommitPartitcipants?

+1, will fix it.

Well, as the file comment of fdwxact.c,
FdwXactRegisterTransaction is called from FDW driver and
F_X_MarkForeignTransactionModified is called from executor. I
think that we should clarify who is responsible to the whole
sequence. Since the state of local tables affects, I suppose
executor is that. Couldn't we do the whole thing within executor
side? I'm not sure but I feel that
F_X_RegisterForeignTransaction can be a part of
F_X_MarkForeignTransactionModified. The callers of
MarkForeignTransactionModified can find whether the table is
involved in 2pc by IsTwoPhaseCommitEnabled interface.

Indeed. We can register foreign servers by executor while FDWs don't
need to register anything. I will remove the registration function so
that FDW developers don't need to call the register function but only
need to provide atomic commit APIs.

if (foreign_twophase_commit == true &&
((MyXactFlags & XACT_FLAGS_FDWNOPREPARE) != 0) )
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot COMMIT a distributed transaction that has operated on foreign server that doesn't support atomic commit")));

The error is emitted when a the GUC is turned off in the
trasaction where MarkTransactionModify'ed. I think that the
number of the variables' possible states should be reduced for
simplicity. For example in the case, once foreign_twopase_commit
is checked in a transaction, subsequent changes in the
transaction should be ignored during the transaction.

I might have not gotten your comment correctly but since the
foreign_twophase_commit is a PGC_USERSET parameter I think we need to
check it at commit time. Also we need to keep participant servers even
when foreign_twophase_commit is off if both max_prepared_foreign_xacts
and max_foreign_xact_resolvers are > 0.

I will post the updated patch in this week.

Attached the updated version patches.

Based on the review comment from Horiguchi-san, I've changed the
atomic commit API so that the FDW developer who wish to support atomic
commit don't need to call the register function. The atomic commit
APIs are following:

* GetPrepareId
* PrepareForeignTransaction
* CommitForeignTransaction
* RollbackForeignTransaction
* ResolveForeignTransaction
* IsTwophaseCommitEnabled

The all APIs except for GetPreapreId is required for atomic commit.

Also, I've changed the foreign_twophase_commit parameter to an enum
parameter based on the suggestion from Robert[1]/messages/by-id/CA+Tgmob4EqxbaMp0e--jUKYT44RL4xBXkPMxF9EEAD+yBGAdxw@mail.gmail.com. Valid values are
'required', 'prefer' and 'disabled' (default). When set to either
'required' or 'prefer' the atomic commit will be used. The difference
between 'required' and 'prefer' is that when set to 'requried' we
require for *all* modified server to be able to use 2pc whereas when
'prefer' we require 2pc where available. So if any of written
participants disables 2pc or doesn't support atomic comit API the
transaction fails. IOW, when 'required' we can commit only when data
consistency among all participant can be left.

Please review the patches.

[1]: /messages/by-id/CA+Tgmob4EqxbaMp0e--jUKYT44RL4xBXkPMxF9EEAD+yBGAdxw@mail.gmail.com

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Mon, Oct 29, 2018 at 10:16 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Wed, Oct 24, 2018 at 9:06 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Oct 23, 2018 at 12:54 PM Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:

Hello.

# It took a long time to come here..

At Fri, 19 Oct 2018 21:38:35 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in <CAD21AoCBf-AJup-_ARfpqR42gJQ_XjNsvv-XE0rCOCLEkT=HCg@mail.gmail.com>

On Wed, Oct 10, 2018 at 1:34 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

...

* Updated docs, added the new section "Distributed Transaction" at
Chapter 33 to explain the concept to users

* Moved atomic commit codes into src/backend/access/fdwxact directory.

* Some bug fixes.

Please reivew them.

I have some comments, with apologize in advance for possible
duplicate or conflict with others' comments so far.

Thank youf so much for reviewing this patch!

0001:

This sets XACT_FLAG_WROTENONTEMPREL when RELPERSISTENT_PERMANENT
relation is modified. Isn't it needed when UNLOGGED tables are
modified? It may be better that we have dedicated classification
macro or function.

I think even if we do atomic commit for modifying the an UNLOGGED
table and a remote table the data will get inconsistent if the local
server crashes. For example, if the local server crashes after
prepared the transaction on foreign server but before the local commit
and, we will lose the all data of the local UNLOGGED table whereas the
modification of remote table is rollbacked. In case of persistent
tables, the data consistency is left. So I think the keeping data
consistency between remote data and local unlogged table is difficult
and want to leave it as a restriction for now. Am I missing something?

The flag is handled in heapam.c. I suppose that it should be done
in the upper layer considering coming pluggable storage.
(X_F_ACCESSEDTEMPREL is set in heapam, but..)

Yeah, or we can set the flag after heap_insert in ExecInsert.

0002:

The name FdwXactParticipantsForAC doesn't sound good for me. How
about FdwXactAtomicCommitPartitcipants?

+1, will fix it.

Well, as the file comment of fdwxact.c,
FdwXactRegisterTransaction is called from FDW driver and
F_X_MarkForeignTransactionModified is called from executor. I
think that we should clarify who is responsible to the whole
sequence. Since the state of local tables affects, I suppose
executor is that. Couldn't we do the whole thing within executor
side? I'm not sure but I feel that
F_X_RegisterForeignTransaction can be a part of
F_X_MarkForeignTransactionModified. The callers of
MarkForeignTransactionModified can find whether the table is
involved in 2pc by IsTwoPhaseCommitEnabled interface.

Indeed. We can register foreign servers by executor while FDWs don't
need to register anything. I will remove the registration function so
that FDW developers don't need to call the register function but only
need to provide atomic commit APIs.

if (foreign_twophase_commit == true &&
((MyXactFlags & XACT_FLAGS_FDWNOPREPARE) != 0) )
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot COMMIT a distributed transaction that has operated on foreign server that doesn't support atomic commit")));

The error is emitted when a the GUC is turned off in the
trasaction where MarkTransactionModify'ed. I think that the
number of the variables' possible states should be reduced for
simplicity. For example in the case, once foreign_twopase_commit
is checked in a transaction, subsequent changes in the
transaction should be ignored during the transaction.

I might have not gotten your comment correctly but since the
foreign_twophase_commit is a PGC_USERSET parameter I think we need to
check it at commit time. Also we need to keep participant servers even
when foreign_twophase_commit is off if both max_prepared_foreign_xacts
and max_foreign_xact_resolvers are > 0.

I will post the updated patch in this week.

Attached the updated version patches.

Based on the review comment from Horiguchi-san, I've changed the
atomic commit API so that the FDW developer who wish to support atomic
commit don't need to call the register function. The atomic commit
APIs are following:

* GetPrepareId
* PrepareForeignTransaction
* CommitForeignTransaction
* RollbackForeignTransaction
* ResolveForeignTransaction
* IsTwophaseCommitEnabled

The all APIs except for GetPreapreId is required for atomic commit.

Also, I've changed the foreign_twophase_commit parameter to an enum
parameter based on the suggestion from Robert[1]. Valid values are
'required', 'prefer' and 'disabled' (default). When set to either
'required' or 'prefer' the atomic commit will be used. The difference
between 'required' and 'prefer' is that when set to 'requried' we
require for *all* modified server to be able to use 2pc whereas when
'prefer' we require 2pc where available. So if any of written
participants disables 2pc or doesn't support atomic comit API the
transaction fails. IOW, when 'required' we can commit only when data
consistency among all participant can be left.

Please review the patches.

Since the previous patch conflicts with current HEAD attached updated
set of patches.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Mon, Oct 29, 2018 at 6:03 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Oct 29, 2018 at 10:16 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Wed, Oct 24, 2018 at 9:06 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Oct 23, 2018 at 12:54 PM Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:

Hello.

# It took a long time to come here..

At Fri, 19 Oct 2018 21:38:35 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in <CAD21AoCBf-AJup-_ARfpqR42gJQ_XjNsvv-XE0rCOCLEkT=HCg@mail.gmail.com>

On Wed, Oct 10, 2018 at 1:34 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

...

* Updated docs, added the new section "Distributed Transaction" at
Chapter 33 to explain the concept to users

* Moved atomic commit codes into src/backend/access/fdwxact directory.

* Some bug fixes.

Please reivew them.

I have some comments, with apologize in advance for possible
duplicate or conflict with others' comments so far.

Thank youf so much for reviewing this patch!

0001:

This sets XACT_FLAG_WROTENONTEMPREL when RELPERSISTENT_PERMANENT
relation is modified. Isn't it needed when UNLOGGED tables are
modified? It may be better that we have dedicated classification
macro or function.

I think even if we do atomic commit for modifying the an UNLOGGED
table and a remote table the data will get inconsistent if the local
server crashes. For example, if the local server crashes after
prepared the transaction on foreign server but before the local commit
and, we will lose the all data of the local UNLOGGED table whereas the
modification of remote table is rollbacked. In case of persistent
tables, the data consistency is left. So I think the keeping data
consistency between remote data and local unlogged table is difficult
and want to leave it as a restriction for now. Am I missing something?

The flag is handled in heapam.c. I suppose that it should be done
in the upper layer considering coming pluggable storage.
(X_F_ACCESSEDTEMPREL is set in heapam, but..)

Yeah, or we can set the flag after heap_insert in ExecInsert.

0002:

The name FdwXactParticipantsForAC doesn't sound good for me. How
about FdwXactAtomicCommitPartitcipants?

+1, will fix it.

Well, as the file comment of fdwxact.c,
FdwXactRegisterTransaction is called from FDW driver and
F_X_MarkForeignTransactionModified is called from executor. I
think that we should clarify who is responsible to the whole
sequence. Since the state of local tables affects, I suppose
executor is that. Couldn't we do the whole thing within executor
side? I'm not sure but I feel that
F_X_RegisterForeignTransaction can be a part of
F_X_MarkForeignTransactionModified. The callers of
MarkForeignTransactionModified can find whether the table is
involved in 2pc by IsTwoPhaseCommitEnabled interface.

Indeed. We can register foreign servers by executor while FDWs don't
need to register anything. I will remove the registration function so
that FDW developers don't need to call the register function but only
need to provide atomic commit APIs.

if (foreign_twophase_commit == true &&
((MyXactFlags & XACT_FLAGS_FDWNOPREPARE) != 0) )
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot COMMIT a distributed transaction that has operated on foreign server that doesn't support atomic commit")));

The error is emitted when a the GUC is turned off in the
trasaction where MarkTransactionModify'ed. I think that the
number of the variables' possible states should be reduced for
simplicity. For example in the case, once foreign_twopase_commit
is checked in a transaction, subsequent changes in the
transaction should be ignored during the transaction.

I might have not gotten your comment correctly but since the
foreign_twophase_commit is a PGC_USERSET parameter I think we need to
check it at commit time. Also we need to keep participant servers even
when foreign_twophase_commit is off if both max_prepared_foreign_xacts
and max_foreign_xact_resolvers are > 0.

I will post the updated patch in this week.

Attached the updated version patches.

Based on the review comment from Horiguchi-san, I've changed the
atomic commit API so that the FDW developer who wish to support atomic
commit don't need to call the register function. The atomic commit
APIs are following:

* GetPrepareId
* PrepareForeignTransaction
* CommitForeignTransaction
* RollbackForeignTransaction
* ResolveForeignTransaction
* IsTwophaseCommitEnabled

The all APIs except for GetPreapreId is required for atomic commit.

Also, I've changed the foreign_twophase_commit parameter to an enum
parameter based on the suggestion from Robert[1]. Valid values are
'required', 'prefer' and 'disabled' (default). When set to either
'required' or 'prefer' the atomic commit will be used. The difference
between 'required' and 'prefer' is that when set to 'requried' we
require for *all* modified server to be able to use 2pc whereas when
'prefer' we require 2pc where available. So if any of written
participants disables 2pc or doesn't support atomic comit API the
transaction fails. IOW, when 'required' we can commit only when data
consistency among all participant can be left.

Please review the patches.

Since the previous patch conflicts with current HEAD attached updated
set of patches.

Rebased and fixed a few bugs.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Thu, Nov 15, 2018 at 7:36 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Oct 29, 2018 at 6:03 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Oct 29, 2018 at 10:16 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Wed, Oct 24, 2018 at 9:06 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Oct 23, 2018 at 12:54 PM Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:

Hello.

# It took a long time to come here..

At Fri, 19 Oct 2018 21:38:35 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in <CAD21AoCBf-AJup-_ARfpqR42gJQ_XjNsvv-XE0rCOCLEkT=HCg@mail.gmail.com>

On Wed, Oct 10, 2018 at 1:34 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

...

* Updated docs, added the new section "Distributed Transaction" at
Chapter 33 to explain the concept to users

* Moved atomic commit codes into src/backend/access/fdwxact directory.

* Some bug fixes.

Please reivew them.

I have some comments, with apologize in advance for possible
duplicate or conflict with others' comments so far.

Thank youf so much for reviewing this patch!

0001:

This sets XACT_FLAG_WROTENONTEMPREL when RELPERSISTENT_PERMANENT
relation is modified. Isn't it needed when UNLOGGED tables are
modified? It may be better that we have dedicated classification
macro or function.

I think even if we do atomic commit for modifying the an UNLOGGED
table and a remote table the data will get inconsistent if the local
server crashes. For example, if the local server crashes after
prepared the transaction on foreign server but before the local commit
and, we will lose the all data of the local UNLOGGED table whereas the
modification of remote table is rollbacked. In case of persistent
tables, the data consistency is left. So I think the keeping data
consistency between remote data and local unlogged table is difficult
and want to leave it as a restriction for now. Am I missing something?

The flag is handled in heapam.c. I suppose that it should be done
in the upper layer considering coming pluggable storage.
(X_F_ACCESSEDTEMPREL is set in heapam, but..)

Yeah, or we can set the flag after heap_insert in ExecInsert.

0002:

The name FdwXactParticipantsForAC doesn't sound good for me. How
about FdwXactAtomicCommitPartitcipants?

+1, will fix it.

Well, as the file comment of fdwxact.c,
FdwXactRegisterTransaction is called from FDW driver and
F_X_MarkForeignTransactionModified is called from executor. I
think that we should clarify who is responsible to the whole
sequence. Since the state of local tables affects, I suppose
executor is that. Couldn't we do the whole thing within executor
side? I'm not sure but I feel that
F_X_RegisterForeignTransaction can be a part of
F_X_MarkForeignTransactionModified. The callers of
MarkForeignTransactionModified can find whether the table is
involved in 2pc by IsTwoPhaseCommitEnabled interface.

Indeed. We can register foreign servers by executor while FDWs don't
need to register anything. I will remove the registration function so
that FDW developers don't need to call the register function but only
need to provide atomic commit APIs.

if (foreign_twophase_commit == true &&
((MyXactFlags & XACT_FLAGS_FDWNOPREPARE) != 0) )
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot COMMIT a distributed transaction that has operated on foreign server that doesn't support atomic commit")));

The error is emitted when a the GUC is turned off in the
trasaction where MarkTransactionModify'ed. I think that the
number of the variables' possible states should be reduced for
simplicity. For example in the case, once foreign_twopase_commit
is checked in a transaction, subsequent changes in the
transaction should be ignored during the transaction.

I might have not gotten your comment correctly but since the
foreign_twophase_commit is a PGC_USERSET parameter I think we need to
check it at commit time. Also we need to keep participant servers even
when foreign_twophase_commit is off if both max_prepared_foreign_xacts
and max_foreign_xact_resolvers are > 0.

I will post the updated patch in this week.

Attached the updated version patches.

Based on the review comment from Horiguchi-san, I've changed the
atomic commit API so that the FDW developer who wish to support atomic
commit don't need to call the register function. The atomic commit
APIs are following:

* GetPrepareId
* PrepareForeignTransaction
* CommitForeignTransaction
* RollbackForeignTransaction
* ResolveForeignTransaction
* IsTwophaseCommitEnabled

The all APIs except for GetPreapreId is required for atomic commit.

Also, I've changed the foreign_twophase_commit parameter to an enum
parameter based on the suggestion from Robert[1]. Valid values are
'required', 'prefer' and 'disabled' (default). When set to either
'required' or 'prefer' the atomic commit will be used. The difference
between 'required' and 'prefer' is that when set to 'requried' we
require for *all* modified server to be able to use 2pc whereas when
'prefer' we require 2pc where available. So if any of written
participants disables 2pc or doesn't support atomic comit API the
transaction fails. IOW, when 'required' we can commit only when data
consistency among all participant can be left.

Please review the patches.

Since the previous patch conflicts with current HEAD attached updated
set of patches.

Rebased and fixed a few bugs.

I got feedbacks regarding transaciton management FDW APIs at Japan
PostgreSQL Developer Meetup[1]https://wiki.postgresql.org/wiki/Japan_PostgreSQL_Developer_Meetup and am considering to change these APIs
to make them consistent with XA interface[2]https://en.wikipedia.org/wiki/X/Open_XA (xa_prepare(),
xa_commit() and xa_rollback()) as follows[3]The current API design I'm proposing has 6 APIs: Prepare, Commit, Rollback, Resolve, IsTwoPhaseEnabled and GetPrepareId. And these APIs are devided based on who executes it..

* FdwXactResult PrepareForeignTransaction(FdwXactState *state, inf flags)
* FdwXactResult CommitForeignTransaction(FdwXactState *state, inf flags)
* FdwXactResult RollbackForeignTransaction(FdwXactState *state, inf flags)
* char *GetPrepareId(TransactionId xid, Oid serverid, Oid userid, int
*prep_id_len)

Where flags set variaous setttings, currently it would contain only
FDW_XACT_FLAG_ONEPHASE that requires FDW to commit in one-phase (i.e.
without preparation). And where *state would contains information
necessary for specifying transaction: serverid, userid, usermappingid
and prepared id. GetPrepareId API is optional. Also I've removed the
two_phase_commit parameter from postgres_fdw options because we can
disable to use two-phase commit protocol for distributed transactions
using by distributed_atomic_commit GUC parameter.

Foreign transactions whose FDW provides both CommitForeignTransaction
API and RollbackForeignTransaction API will be managed by the global
transaction manager automatically. In addition, if the FDW also
provide PrepareForeignTransaction API it will participate to two-phase
commit protocol as a participant. So the existing FDWs that don't
provide transaction management FDW APIs can continue to work as before
even though this patch get committed.

The one point I'm concerned about this API design would be that since
both CommitForeignTransaction API and RollbackForeignTransaction API
will be used by two different kinds of process (backend and
transaction resolver processes), it might be hard to understand them
correctly for FDW developers.

I'd like to define new APIs so that FDW developers don't get confused.
Feedback is very welcome.

[1]: https://wiki.postgresql.org/wiki/Japan_PostgreSQL_Developer_Meetup
[2]: https://en.wikipedia.org/wiki/X/Open_XA
[3]: The current API design I'm proposing has 6 APIs: Prepare, Commit, Rollback, Resolve, IsTwoPhaseEnabled and GetPrepareId. And these APIs are devided based on who executes it.
Rollback, Resolve, IsTwoPhaseEnabled and GetPrepareId. And these APIs
are devided based on who executes it.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Hello,

The patch needs rebase as it doesn't apply to the current master. I applied
it
to the older commit to test it. It worked fine so far.

I found one bug though which would cause resolver to finish by timeout even
though there are unresolved foreign transactions in the list. The
`fdw_xact_exists()` function expects database id as the first argument and
xid
as the second. But everywhere it is called arguments specified in the
different
order (xid first, then dbid). Also function declaration in header doesn't
match its definition.

There are some other things I found.
* In `FdwXactResolveAllDanglingTransactions()` variable `n_resolved` is
declared as bool but used as integer.
* In fdwxact.c's module comment there are
`FdwXactRegisterForeignTransaction()`
and `FdwXactMarkForeignTransactionModified()` functions mentioned that are
not there anymore.
* In documentation (storage.sgml) there is no mention of `pg_fdw_xact`
directory.

Couple of stylistic notes.
* In `FdwXactCtlData struct` there are both camel case and snake case naming
used.
* In `get_fdw_xacts()` `xid != InvalidTransactionId` can be replaced with
`TransactionIdIsValid(xid)`.
* In `generate_fdw_xact_identifier()` the `fx` prefix could be a part of
format
string instead of being processed by `sprintf` as an extra argument.

I'll continue looking into the patch. Thanks!

On Tue, Nov 20, 2018 at 12:18 PM Masahiko Sawada <sawada.mshk@gmail.com>
wrote:

On Tue, Jan 29, 2019 at 5:47 PM Ildar Musin <ildar@adjust.com> wrote:

Hello,

The patch needs rebase as it doesn't apply to the current master. I applied it
to the older commit to test it. It worked fine so far.

Thank you for testing the patch!

I found one bug though which would cause resolver to finish by timeout even
though there are unresolved foreign transactions in the list. The
`fdw_xact_exists()` function expects database id as the first argument and xid
as the second. But everywhere it is called arguments specified in the different
order (xid first, then dbid). Also function declaration in header doesn't
match its definition.

Will fix.

There are some other things I found.
* In `FdwXactResolveAllDanglingTransactions()` variable `n_resolved` is
declared as bool but used as integer.
* In fdwxact.c's module comment there are `FdwXactRegisterForeignTransaction()`
and `FdwXactMarkForeignTransactionModified()` functions mentioned that are
not there anymore.
* In documentation (storage.sgml) there is no mention of `pg_fdw_xact`
directory.

Couple of stylistic notes.
* In `FdwXactCtlData struct` there are both camel case and snake case naming
used.
* In `get_fdw_xacts()` `xid != InvalidTransactionId` can be replaced with
`TransactionIdIsValid(xid)`.
* In `generate_fdw_xact_identifier()` the `fx` prefix could be a part of format
string instead of being processed by `sprintf` as an extra argument.

I'll incorporate them at the next patch set.

I'll continue looking into the patch. Thanks!

Thanks. Actually I'm updating the patch set, changing API interface as
I proposed before and improving the document and README. I'll submit
the latest patch next week.

--
Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Thu, Jan 31, 2019 at 11:09:09AM +0100, Masahiko Sawada wrote:

Thanks. Actually I'm updating the patch set, changing API interface as
I proposed before and improving the document and README. I'll submit
the latest patch next week.

Cool, I have moved the patch to next CF.
--
Michael

On Thu, Jan 31, 2019 at 7:09 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Jan 29, 2019 at 5:47 PM Ildar Musin <ildar@adjust.com> wrote:

Hello,

The patch needs rebase as it doesn't apply to the current master. I applied it
to the older commit to test it. It worked fine so far.

Thank you for testing the patch!

I found one bug though which would cause resolver to finish by timeout even
though there are unresolved foreign transactions in the list. The
`fdw_xact_exists()` function expects database id as the first argument and xid
as the second. But everywhere it is called arguments specified in the different
order (xid first, then dbid). Also function declaration in header doesn't
match its definition.

Will fix.

There are some other things I found.
* In `FdwXactResolveAllDanglingTransactions()` variable `n_resolved` is
declared as bool but used as integer.
* In fdwxact.c's module comment there are `FdwXactRegisterForeignTransaction()`
and `FdwXactMarkForeignTransactionModified()` functions mentioned that are
not there anymore.
* In documentation (storage.sgml) there is no mention of `pg_fdw_xact`
directory.

Couple of stylistic notes.
* In `FdwXactCtlData struct` there are both camel case and snake case naming
used.
* In `get_fdw_xacts()` `xid != InvalidTransactionId` can be replaced with
`TransactionIdIsValid(xid)`.
* In `generate_fdw_xact_identifier()` the `fx` prefix could be a part of format
string instead of being processed by `sprintf` as an extra argument.

I'll incorporate them at the next patch set.

I'll continue looking into the patch. Thanks!

Thanks. Actually I'm updating the patch set, changing API interface as
I proposed before and improving the document and README. I'll submit
the latest patch next week.

Sorry for the very late. Attached updated version patches.

The basic mechanism has not been changed since the previous version.
But the updated version patch uses the single wait queue instead of
two queues (active and retry) which were used in the previous version.

Every backends processes has a timestamp in PGPROC
(fdwXactNextResolutionTs), that is the time when they expect to be
processed by foreign resolver process at. Entries in the wait queue is
ordered by theirs timestamps. The wait queue and timestamp are used
after a backend process prepared all transactions on foreign servers
and wait for all of them to be resolved.

Backend processes who are committing/aborting the distributed
transaction insert itself to the wait queue
(FdwXactRslvCtl->fdwxact_queue) with the current timestamp, and then
request to launch a new resolver process if not launched yet. If there
is resolver connecting to the same database they just set its latch.
The wait queue is protected by LWLock FdwXactResolutionLock. Then the
backend sleep until either user requests to cancel (press ctrl-c) or
waken up by resolver process.

Foreign resolver process continue to poll the wait queue, checking if
there is any waiter on the database that the resolver process connects
to. If there is a waiter, fetches it and check its timestamp. If the
current timestamp goes over its timestamp, the resolver process start
to resolve all foreign transactions. Usually backends processes insert
itself to wait queue first then wake up the resolver and they use the
same wall-clock, so the resolver can fetch the waiter just inserted.
Once all foreign transactions are resolved, the resolver process
delete the backend entry from the wait queue, and then wake up the
waiting backend.

On failure during foreign transaction resolution, while the backend is
still sleeping, the resolver process removes and inserts the backend
with the new timestamp (its timestamp
foreign_transaction_resolution_interval) to appropriate position in
the wait queue. This mechanism ensures that a distributed transaction
is resolved as soon as the waiter inserted while ensuring that the
resolver can retry to resolve the failed foreign transactions at a
interval of foreign_transaction_resolution_interval time.

For handling in-doubt transactions, I've removed the automatically
foreign transaction resolution code from the first version patch since
it's not essential feature and we can add it later. Therefore user
needs to resolve unresolved foreign transactions manually using by
pg_resolve_fdwxacts() function in three cases: where the foreign
server crashed or we lost connectibility to it during preparing
foreign transaction, where the coordinator node crashed during
preparing/resolving the foreign transaction and where user canceled to
resolve the foreign transaction.

For foreign transaction resolver processes, they exit if they don't
have any foreign transaction to resolve longer than
foreign_transaction_resolver_timeout. Since we cannot drop a database
while a resolver process is connecting to we can stop it call by
pg_stop_fdwxact_resolver() function.

The comment at top of fdwxact.c file describes about locking mechanism
and recovery, and src/backend/fdwxact/README descries about status
transition of FdwXact.

Also the wiki page[1]https://wiki.postgresql.org/wiki/Atomic_Commit_of_Distributed_Transactions describes how to use this feature with some examples.

[1]: https://wiki.postgresql.org/wiki/Atomic_Commit_of_Distributed_Transactions

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center