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

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.
>

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