On Wed, 8 Mar 2017 16:49:33 +0100
Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
> I did a review of this patch
First, thank you for you effort. I already begin to think that nobody
is really interesting in PL/Tcl stuff.
> 1. This functionality has sense and nobody was against this feature.
>
> 2. This patch does what is proposed - it introduce new TCL function
> that wraps PostgreSQL subtransaction
>
> 3. This patch is really simple due massive using subtransactions
> already - every SPI called from TCL is wrapped to subtransaction.
>
> 4. A documentation is good - although I am not sure if it is well
> structured - is <sect2> level necessary? Probably there will not be
> any other similar command.
You are right. At least sect2 can be added later whenever this second
command will appear.
> 5. There are a basic regress tests, and all tests passed, but I miss a
> path, where subtransaction is commited - now rollback is every time
Really, I haven't found such tests in PL/Python suite, so I haven't
translated it to Tcl.
It might be good idea to add such test.
> 6. The code has some issues with white chars
>
> 7. I don't understand why TopMemoryContext is used there? Maybe some
> already used context should be there.
>
> +<->BeginInternalSubTransaction(NULL);
> +<->MemoryContextSwitchTo(TopTransactionContext);
> +<->
I've copied this code from PL/Python subtransaction object and
it has following comment: /* Be sure that cells of explicit_subtransactions list are long-lived */
But in Tcl case wi don't have to maintain our own stack in the form of
list. We use C-language stack and keep our data in the local variables.
So, probably changing of memory contexts is not necessary at all.
But it require a bit more investigation and testing.
With best regards, Victor
-- Victor Wagner <vitus@wagner.pp.ru>
