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

On Wed, Oct 10, 2018 at 1:34 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Wed, Oct 3, 2018 at 6:02 PM Chris Travers <chris(dot)travers(at)adjust(dot)com> wrote:
> >
> >
> >
> > On Wed, Oct 3, 2018 at 9:41 AM Chris Travers <chris(dot)travers(at)gmail(dot)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