On 26/12/2018 09:20, Fabien COELHO wrote:
> I try to avoid global variables when possible as a principle, because I
> paid to learn that they are bad:-) Maybe I'd do a local struct, declare an
> instance within the function, and write two functions to dump/restore the
> transaction status variables into the referenced struct. Maybe this is not
> worth the effort.
Those are reasonable alternatives, I think, but it's a bit overkill in
this case.
>>> About the tests: I'd suggest to use more options on the different tests,
>>> eg SERIALIZABLE, READ ONLY… Also ISTM that tests should show
>>> transaction_read_only value as well.
>>
>> OK, updated a bit. (Using READ ONLY doesn't work because then you can't
>> write anything inside the transaction.)
>
> Sure. Within a read-only tx, it could check that transaction_read_only is
> on, and still on when chained, though.
I think the tests prove the point that the values are set and unset and
reset in various scenarios. We don't need to test every single
combination, that's not the job of this patch.
> The documentation should probably tell in the compatibility sections that
> AND CHAIN is standard conforming.
Good point. Updated that.
> In the automatic rollback tests, maybe I'd insert 3 just once, and use
> another value for the chained transaction.
done
> Tests may eventually vary BEGIN/START, COMMIT/END, ROLLBACK/ABORT.
Those work on the parser level, so don't really affect this patch. It
would make the tests more confusing to read, I think.
> As you added a SAVEPOINT, maybe I'd try rollbacking to it.
That would error out and invalidate the subsequent tests, so it's not
easily possible. We could write a totally separate test for it, but I'm
not sure what that would be proving.
Updated patches attached.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services