Thread: [HACKERS] Explicit subtransactions for PL/Tcl

Collegues!

Recently I've found out that PL/Python have very nice feature - explicit
subtransaction object, which allows to execute block of code in the
context of subtransaction.

I've quite surprised that other PL languages, shipped with PostgreSQL do
not have such useful construction. 

If it might require considerable trickery to add such functionality into
PL/Perl, Tcl allows to add new control stuctures very easily.

I'm attaching the patch which implements subtransaction command for
PL/Tcl which does almost same as PL/Python plpy.subtransction context
manager object does: executes a block of Tcl code in the context of
subtransaction, rolls subtransaction back if error occures and commits
it otherwise.

It looks like

subtransaction {
     ...some Tcl code...
}

Typically one would use it inside Tcl catch statement:

if [catch {
    subtransaction {
        spi_exec "insert into..."
                ...
    }
} errormsg] {
   # Handle an error
}

See documentation and tests included in the patch for more complete
examples.

Just like corresponding Python construction, this command doesn't
replace language  builtin exception handling, just adds subtransaction 
support to it.

Patch includes sufficiently less tests than python subtransaction tests,
because Tcl implementation is way simpler than python one, and doesn't
have syntactic variatons which depend on language version.

Also entering and exiting subtransactions are in the same piece of code
rather than in separate __enter__ and __exit__ methods as in Python, so
there is no possibility to call exit without enter or vice versa.

-- 
                                   Victor Wagner <vitus@wagner.pp.ru>

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sun, 8 Jan 2017 20:57:50 +0300
Victor Wagner <vitus@wagner.pp.ru> wrote:

> Collegues!
> 
> Recently I've found out that PL/Python have very nice feature -
> explicit subtransaction object, which allows to execute block of code
> in the context of subtransaction.
> 
[skip]

> 
> I'm attaching the patch which implements subtransaction command for

Sorry, unfortunately attached empty file instead of patch
-- 
                                   Victor Wagner <vitus@wagner.pp.ru>

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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>

On Wed, 8 Mar 2017 16:49:33 +0100
Pavel Stehule <pavel.stehule@gmail.com> wrote:

> 
> I did a review of this patch
> 
I'm attaching new version of patch with the issues pointed by you fixed.

> 
> 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.

Removed <sect2>. Although it is questionable - now there is no
subsection with name of the command in the title. But surrounding
section describes only one command,.



> 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

Added test with successful subtransaction commit. Just to be sure it is
really commited.

 
> 6. The code has some issues with white chars

Fixed where I've found it. Now, hopefully my code uses same identation
rules as other pltcl.c


> 7. I don't understand why TopMemoryContext is used there? Maybe some
> already used context should be there.
> 
> +<->BeginInternalSubTransaction(NULL);
> +<->MemoryContextSwitchTo(TopTransactionContext);
> +<->

It turns out that was really an error in the first version of patch.
This context manipulation was copied from PL/Python context manager by
mistake.

PL/Python changes to TopTransactionContext in order to manage
stack of subtransaction data (which we don't need in Tcl, because we
can use C language stack and store neccessary data in function
automatic variables. Really python doesn't need this stack to, because
Python context manager is a python language object and can keep state
inside it).

Although we still need to keep MemoryContext and ResourceOwner and
restore them upon  transaction finish.

        With best regards, Victor

-- 
        Victor Wagner <vitus@wagner.pp.ru>
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, 9 Mar 2017 09:25:14 +0100
Pavel Stehule <pavel.stehule@gmail.com> wrote:


> >
> is this patch complete? I don't see new regress tests

Oh, really! I've forgot that git diff doesn't include files which are
not added into git.

So, no old regress tests as well.

Sorry for posting incomplete patch.

Attached fixed version of patch with regress tests and couple more
whitespace issues fixed.

                 With best regards, Victor

--
                Victor Wagner <vitus@wagner.pp.ru>



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, 9 Mar 2017 11:12:09 +0100
Pavel Stehule <pavel.stehule@gmail.com> wrote:


> the regress tests is unstable
> 
> the proc name has mutable pid
> 
> !     (procedure "__PLTcl_proc_16503" line 3)
>       invoked from within
> ! "__PLTcl_proc_16503 SPI"
> 
> Regards

Really, I don't know what can be done with it, short of rewriting all
tests as tap-tests.

Definitely this patch is not the right place for reversing desing
decision of PL/Tcl authors to add a numeric suffix to the proc names.
(it is not PID. It is OID of the function).

Of course, I can catch all the errors inside Tcl code and return
just message, but it would sufficiently narrow test functionality.

Now test demonstrate how errors uncaught on the Tcl level interact with
postgresql error system.


                With best regards, Victor

--                Victor Wagner <vitus@wagner.pp.ru>

On Thu, 9 Mar 2017 12:04:31 +0100
Pavel Stehule <pavel.stehule@gmail.com> wrote:


> > Now test demonstrate how errors uncaught on the Tcl level interact
> > with postgresql error system.
> >
> 
> you can catch the exception outside and write own message

OK, here is patch with tests which don't depend on function OIDs

They ignore stack trace ($::errorinfo global variable) completely,
and analyze just error message.


-- 
                                   Victor Wagner <vitus@wagner.pp.ru>

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Pavel Stehule <pavel.stehule@gmail.com> writes:
> I'll mark this patch as ready for commiter

I've pushed this after mostly-cosmetic cleanup.  One thing I changed
that's not cosmetic is to put
MemoryContextSwitchTo(oldcontext);

after the BeginInternalSubTransaction call.  I see there was some
discussion upthread about what to do there, but I think this is the
correct answer because it corresponds to what pltcl_subtrans_begin
does.  That means that the execution environment of the called Tcl
fragment will be the same as, eg, the loop_body in spi_exec.  I do not
think we want it to be randomly different from those pre-existing cases.

Now, I believe that the MemoryContextSwitchTo in pltcl_subtrans_begin
was probably just cargo-culted in there from similar code in plpgsql.
Since pltcl doesn't rely on CurrentMemoryContext to anywhere near the
same degree that plpgsql does, it's possible that we could drop the
MemoryContextSwitchTo in both places, and just let the called code
fragments run in the subtransaction's memory context.  But I'm not
convinced of that, and in any case it ought to be undertaken as a
separate patch.

Some other comments:

* Whitespace still wasn't very much per project conventions.
I fixed that by running it through pgindent.

* The golden rule for code style and placement is "make your patch
look like it was always there".  Dropping new code at the tail end
of the file is seldom a good avenue to that.  I stuck it after
pltcl_SPI_lastoid, on the grounds that it should be with the other
Tcl command functions and should respect the (mostly) alphabetical
order of those functions.  Likewise I adopted a header comment
format in keeping with the existing nearby functions.

* I whacked the SGML docs around pretty thoroughly.  That addition
wasn't respecting the style of the surrounding text either as to
markup or indentation, and it had some other issues like syntax
errors in the example functions.
        regards, tom lane