Hello Peter,
Shouldn't psql tab completion be updated as well?
Done. (I only did the AND CHAIN, not the AND NO CHAIN.)
Sure, that is the useful part.
I must admit that do not like much the three global variables &
Save/Restore functions. I'd suggest saving directly into 3 local variables
in function CommitTransactionCommand, and restoring them when needed. Code
should not be longer. I'd would not bother to skip saving when not
chaining.
We're also using those functions in the 0002 patch.
Hmmm. I have not looked at the second one yet.
Should we just repeat the code three times, or use macros?
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.
Copying & comparing nodes are updated. Should making, outing and reading
nodes also be updated?
TransactionStmt isn't covered by the node serialization functions, so I
didn't see anything to update. What did you have in mind?
Sigh. I had in mind that the serialization feature would work with all
possible nodes, not just some of them… which seems quite naïve. The whole
make/copy/cmp/in/out functions depress me, all this verbose code should be
automatically generated from struct declarations. I'm pretty sure there
are hidden bugs in there.
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.
Updated patch attached.
First patch applies cleanly, compiles, make check ok.
Further remarks, distinct from the above comments and suggestions:
The documentation should probably tell in the compatibility sections that
AND CHAIN is standard conforming.
In the automatic rollback tests, maybe I'd insert 3 just once, and use
another value for the chained transaction.
Tests may eventually vary BEGIN/START, COMMIT/END, ROLLBACK/ABORT.
As you added a SAVEPOINT, maybe I'd try rollbacking to it.
--
Fabien.
