On Tue, May 16, 2017 at 9:39 PM, Robert Haas <robertmh...@gmail.com> wrote: > On Sun, May 7, 2017 at 11:54 AM, Robert Haas <robertmh...@gmail.com> wrote: >> I'm having second thoughts based on some more experimentation I did >> this morning. I'll update again once I've had a bit more time to poke >> at it. > > So the issue that I noticed here is that this problem really isn't > confined to abort processing. If we ROLLBACK TO SAVEPOINT or ABORT > TRANSACTION on an open connection, we really do not know what the > state of that connection is until we get an acknowledgement that the > command completed successfully. The patch handles that. However, the > same is true if we're sending a SAVEPOINT or RELEASE SAVEPOINT > command, and the patch that I posted doesn't do anything about those > cases. I think it would be best to fix all transaction control > commands in a symmetric manner. > > Concretely, I think we should replace the abort_cleanup_incomplete > flag from my previous patch with a changing_xact_state flag and set > that flag around all transaction state changes, clearing it when such > changes have succeeded. On error, the flag remains set, so we know > that the state of that connection is unknown and that we must close it > (killing outer transaction levels as needed). > > Thoughts?
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. I ran the regress test for postgres_fdw and it went on fine. 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; Also, by any chance should we add a test-case for this? -- Regards, Rafia Sabih EnterpriseDB: http://www.enterprisedb.com/
improve-pgfdw-abort-behavior-v3.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
