On Fri, Jun 2, 2017 at 3:48 AM, Rafia Sabih
<rafia.sabih@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
