Re: [HACKERS] Explicit subtransactions for PL/Tcl

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>