Re: Explicit subtransactions for PL/Tcl

On Wed, 8 Mar 2017 16:49:33 +0100
Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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(at)wagner(dot)pp(dot)ru>