Hallo Peter,
> Updated patch. I have squashed the two previously separate patches
> together in this one.
Ok.
>> I do not understand the value of the SAVEPOINT in the tests.
>
> The purpose of the SAVEPOINT in the test is because it exercises
> different switch cases in CommitTransactionCommand() and
> AbortCurrentTransaction(). It's not entirely comprehensible from the
> outside, but code coverage analysis confirms it.
Ok.
>> 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.
>
> I have updated the SPI interface with your suggestions. I agree it's
> better that way.
Patch applies cleanly, compiles, make check ok, doc build ok.
Minor remarks:
In "xact.c", maybe I'd assign blockState in the else branch, instead of
overriding it?
About the static _SPI_{commit,rollback} functions: I'm fine with these
functions, but I'm not sure about their name. Maybe
_SPI_chainable_{commit,rollback} would be is clearer about their content?
Doc looks clear to me. ISTM "chain" should be added as an index term?
Tests look ok. Maybe I'd have series with mixed commit & rollback, instead
of only commit & only rollback?
--
Fabien.
