On Wed, Oct 3, 2018 at 6:02 PM Chris Travers <chris.trav...@adjust.com> wrote: > > > > On Wed, Oct 3, 2018 at 9:41 AM Chris Travers <chris.trav...@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
