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

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.

Regards,

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