Обсуждение: [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
Вложения
Hi
2017-01-08 18:57 GMT+01:00 Victor Wagner <vitus@wagner.pp.ru>:
+<->BeginInternalSubTransaction(NULL);
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.
I did a review of this patch
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.
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
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.
+<->MemoryContextSwitchTo(TopTransactionContext);
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
Вложения
2017-03-09 7:48 GMT+01:00 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.
is this patch complete? I don't see new regress tests
Regards
Pavel
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
Вложения
Hi
2017-03-09 10:25 GMT+01:00 Victor Wagner <vitus@wagner.pp.ru>:
! (procedure "__PLTcl_proc_16503" line 3)
invoked from within
! "__PLTcl_proc_16503 SPI"
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.
the regress tests is unstable
the proc name has mutable pid
invoked from within
! "__PLTcl_proc_16503 SPI"
Regards
Pavel
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>
2017-03-09 11:45 GMT+01:00 Victor Wagner <vitus@wagner.pp.ru>:
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.
you can catch the exception outside and write own message
Pavel
--
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 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
Вложения
2017-03-10 20:31 GMT+01:00 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.
all tests passed
I have not any other objections
I'll mark this patch as ready for commiter
Regards
Pavel
--
Victor Wagner <vitus@wagner.pp.ru>
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