On Fri, Jun 2, 2017 at 3:48 AM, Rafia Sabih <rafia.sa...@enterprisedb.com> wrote: > I tried following the sketch stated above, and here's what I added to > v2 of the patch. In begin_remote_xact when savepoint is send to the > remote server through do_sql_command I set the changing_xact_state > flag and when it successfully returns from there the flag is unset. > Similar behaviour is followed in pgfdw_subxact_callback for release > savepoint. Additionally, I added this flag set-unset for start > transaction command in begin_remote_xact. Enlighten me if I've > msised/misunderstood something here.
You didn't catch all of the places where do_sql_command() is used to change the transaction state, and you didn't update the comments to match the code changes. > I ran the regress test for > postgres_fdw and it went on fine. Well, that's good as far as it goes, but it doesn't prove much, since this patch mostly changes the behavior when there are errors or interrupts involved, and that's not going to happen in the regression tests. > I have a couple of concerns here, since there is only flag required > for the purpose, it's name to be changing_xact_state doesn't suit at > some places particularly in pgfdw_subxact_callback, not sure should > change comment or variable name > > /* Disarm abort_cleanup_incomplete if it all worked. */ > + entry->changing_xact_state = abort_cleanup_failure; Seems pretty obvious that would need to be updated, at least insofar as making the comment match the new structure member name. I mean, there's often some question regarding the degree to which existing comments should be updated, but it hardly makes sense to add a new comment that isn't consistent with the rest of the patch, right? > Also, by any chance should we add a test-case for this? I don't see how to do that. It could possibly be done with the TAP framework, but that exceeds my abilities. Here's an updated patch with a bunch of cosmetic fixes, plus I adjusted things so that do_sql_command() is more interruptible. I tested it manually and it seems to work OK. I'll commit and back-patch this version, barring objections or further suggestions for improvement. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
improve-pgfdw-abort-behavior-v4.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers