Hello Peter,
>> 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.
Hmmm... I've been quite unhappy with the status of non regression tests
for a long time, it seems that many contributors take the same approach.
ISTM that your patch saves three states, so I'd check that they are indeed
saved and restored, esp with non default values. I've noticed that you
change the default to read-only with a SET, but then this new default is
not tested afterwards, it is switched to READ WRITE on each transaction.
>> 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.
Hmmm. Then why put a savepoint if it is not really used?
> Updated patches attached.
First patch applies cleanly, compiles, make check ok.
I do not understand the value of the SAVEPOINT in the tests.
I wish there was a also read-only transaction test.
Otherwise I'm okay with this patch.
About the second patch, I'm still unhappy with functions named commit &
rollback doing something else, which result in somehow strange code, where
you have to guess that the transaction is restarted in all cases, either
within the commit function or explicitely.
SPI_commit/rollback(chain);
if (!chain) SPI_start_transaction();
I think that the PL codes would be clearer with something like:
if (chain)
SPI_commit/rollback_and_chain();
else
{
SPI_commit/rollback();
SPI_start_transation();
}
And there would be no need to change existing SPI_commit/rollback calls to
add a false argument, for those PLs which do not support the extension
(yet).
--
Fabien.