Re: [HACKERS] Explicit subtransactions for PL/Tcl

[Tom Lane]

Pavel Stehule  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


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

Re: [HACKERS] Explicit subtransactions for PL/Tcl

[Pavel Stehule]

2017-03-10 20:31 GMT+01:00 Victor Wagner :

> On Thu, 9 Mar 2017 12:04:31 +0100
> Pavel Stehule  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 
>

Re: [HACKERS] Explicit subtransactions for PL/Tcl

[Victor Wagner]

On Thu, 9 Mar 2017 12:04:31 +0100
Pavel Stehule  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 
diff --git a/doc/src/sgml/pltcl.sgml b/doc/src/sgml/pltcl.sgml
index ad216dd..87bc4ad 100644
--- a/doc/src/sgml/pltcl.sgml
+++ b/doc/src/sgml/pltcl.sgml
@@ -901,6 +901,79 @@ if {[catch { spi_exec $sql_command }]} {
  is a global variable.)
 

+   
+   Explicit Subtransactions
+  
+   Recovering from errors caused by database access as described in
+can lead to an undesirable
+   situation where some operations succeed before one of them fails,
+   and after recovering from that error the data is left in an
+   inconsistent state.  PL/Tcl offers a solution to this problem in
+   the form of explicit subtransactions.
+  
+   
+Consider a function that implements a transfer between two
+accounts:
+
+CREATE FUNCTION transfer_funds() RETURNS void AS $$
+if [catch {
+spi_exec "UPDATE accounts SET balance = balance - 100 WHERE account_name = 'joe'"
+spi_exec "UPDATE accounts SET balance = balance + 100 WHERE account_name = 'mary'"
+} errormsg] {
+set result [format "error transferring funds: %s" $errormsg ]
+} else {
+set result "funds transferred correctly"
+}
+set plan [spi_prepare "INSERT INTO operations (result) VALUES ($1)"]
+spi_execp -count 1 $plan, [list $result)
+$$ LANGUAGE pltclu;
+
+If the second UPDATE statement results in an
+exception being raised, this function will report the error, but
+the result of the first UPDATE will
+nevertheless be committed.  In other words, the funds will be
+withdrawn from Joe's account, but will not be transferred to
+Mary's account.
+   
+   
+To avoid such issues, you can wrap your
+spi_exec calls in an explicit
+subtransaction.  The PL/Tcl provides a
+commmand subtransaction to manage explicit
+	subtransactions.
+ Using explicit subtransactions
+we can rewrite our function as:
+
+CREATE FUNCTION transfer_funds2() RETURNS void AS $$
+if [catch {
+	subtransaction {
+spi_exec "UPDATE accounts SET balance = balance - 100 WHERE account_name = 'joe'"
+spi_exec "UPDATE accounts SET balance = balance + 100 WHERE account_name = 'mary'"
+	}
+	} errormsg] {
+		set result [format "error transferring funds: %s" $errormsg]
+	} else {
+   set result "funds transferred correctly"
+	}
+set plan  [spi_prepare "INSERT INTO operations (result) VALUES ($1)"]
+spi_execp $plan, [list $result]
+$$ LANGUAGE pltclu;
+
+Note that the use of catch is still
+required.  Otherwise the exception would propagate to the top of
+the  stack and would cause the whole function to abort with
+a PostgreSQL error, so that the
+operations table would not have any row
+inserted into it.  The subtransaction command does not
+trap errors, it only assures that all database operations executed
+inside its scope will be atomically committed or rolled back.  A
+rollback of the subtransaction block occurs on any kind of
+exception exit, not only ones caused by errors originating from
+database access.  A regular Tcl exception raised inside an
+explicit subtransaction block would also cause the subtransaction
+to be rolled back.
+   
+  
 

 PL/Tcl Configuration
diff --git a/src/pl/tcl/Makefile b/src/pl/tcl/Makefile
index 1096c4f..b6b6b19 100644
--- a/src/pl/tcl/Makefile
+++ b/src/pl/tcl/Makefile
@@ -28,7 +28,7 @@ DATA = pltcl.control pltcl--1.0.sql pltcl--unpackaged--1.0.sql \
pltclu.control pltclu--1.0.sql pltclu--unpackaged--1.0.sql
 
 REGRESS_OPTS = --dbname=$(PL_TESTDB) --load-extension=pltcl
-REGRESS = pltcl_setup pltcl_queries pltcl_start_proc pltcl_unicode
+REGRESS = pltcl_setup pltcl_queries pltcl_start_proc pltcl_unicode pltcl_subxact
 
 # Tcl on win32 ships with import libraries only for Microsoft Visual C++,
 # which are not compatible with mingw gcc. Therefore we need to build a
diff --git a/src/pl/tcl/expected/pltcl_subxact.out b/src/pl/tcl/expected/pltcl_subxact.out
new file mode 100644
index 000..3ea3ef3
--- /dev/null
+++ b/src/pl/tcl/expected/pltcl_subxact.out
@@ -0,0 +1,146 @@
+--
+-- Test explicit subtransactions
+--
+CREATE TABLE subtransaction_tbl (
+i integer
+);
+-- test subtransaction successfully commited
+CREATE FUNCTION subtransaction_ctx_success() RETURNS text
+AS $$
+	spi_exec "INSERT INTO subtransaction_tbl VALUES(1)"
+subtransaction {
+		spi_exec "INSERT INTO subtransaction_tbl VALUES(2)"
+	}
+$$ LANGUAGE pltcl;
+BEGIN;
+INSERT INTO subtransaction_tbl VALUES(0);
+SELECT subtransaction_ctx_success();

Re: [HACKERS] Explicit subtransactions for PL/Tcl

[Pavel Stehule]

2017-03-09 11:45 GMT+01:00 Victor Wagner :

> On Thu, 9 Mar 2017 11:12:09 +0100
> Pavel Stehule  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

>
>
>
>  With best regards, Victor
>
> --
>  Victor Wagner 
>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Re: [HACKERS] Explicit subtransactions for PL/Tcl

[Victor Wagner]

On Thu, 9 Mar 2017 11:12:09 +0100
Pavel Stehule  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 



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

Re: [HACKERS] Explicit subtransactions for PL/Tcl

[Pavel Stehule]

Hi

2017-03-09 10:25 GMT+01:00 Victor Wagner :

> On Thu, 9 Mar 2017 09:25:14 +0100
> Pavel Stehule  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

! (procedure "__PLTcl_proc_16503" line 3)
  invoked from within
! "__PLTcl_proc_16503 SPI"

Regards

Pavel


>  With best regards, Victor
>
> --
> Victor Wagner 
>
>
>

Re: [HACKERS] Explicit subtransactions for PL/Tcl

[Victor Wagner]

On Thu, 9 Mar 2017 09:25:14 +0100
Pavel Stehule  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 


diff --git a/doc/src/sgml/pltcl.sgml b/doc/src/sgml/pltcl.sgml
index ad216dd..87bc4ad 100644
--- a/doc/src/sgml/pltcl.sgml
+++ b/doc/src/sgml/pltcl.sgml
@@ -901,6 +901,79 @@ if {[catch { spi_exec $sql_command }]} {
  is a global variable.)
 

+   
+   Explicit Subtransactions
+  
+   Recovering from errors caused by database access as described in
+can lead to an undesirable
+   situation where some operations succeed before one of them fails,
+   and after recovering from that error the data is left in an
+   inconsistent state.  PL/Tcl offers a solution to this problem in
+   the form of explicit subtransactions.
+  
+   
+Consider a function that implements a transfer between two
+accounts:
+
+CREATE FUNCTION transfer_funds() RETURNS void AS $$
+if [catch {
+spi_exec "UPDATE accounts SET balance = balance - 100 WHERE account_name = 'joe'"
+spi_exec "UPDATE accounts SET balance = balance + 100 WHERE account_name = 'mary'"
+} errormsg] {
+set result [format "error transferring funds: %s" $errormsg ]
+} else {
+set result "funds transferred correctly"
+}
+set plan [spi_prepare "INSERT INTO operations (result) VALUES ($1)"]
+spi_execp -count 1 $plan, [list $result)
+$$ LANGUAGE pltclu;
+
+If the second UPDATE statement results in an
+exception being raised, this function will report the error, but
+the result of the first UPDATE will
+nevertheless be committed.  In other words, the funds will be
+withdrawn from Joe's account, but will not be transferred to
+Mary's account.
+   
+   
+To avoid such issues, you can wrap your
+spi_exec calls in an explicit
+subtransaction.  The PL/Tcl provides a
+commmand subtransaction to manage explicit
+	subtransactions.
+ Using explicit subtransactions
+we can rewrite our function as:
+
+CREATE FUNCTION transfer_funds2() RETURNS void AS $$
+if [catch {
+	subtransaction {
+spi_exec "UPDATE accounts SET balance = balance - 100 WHERE account_name = 'joe'"
+spi_exec "UPDATE accounts SET balance = balance + 100 WHERE account_name = 'mary'"
+	}
+	} errormsg] {
+		set result [format "error transferring funds: %s" $errormsg]
+	} else {
+   set result "funds transferred correctly"
+	}
+set plan  [spi_prepare "INSERT INTO operations (result) VALUES ($1)"]
+spi_execp $plan, [list $result]
+$$ LANGUAGE pltclu;
+
+Note that the use of catch is still
+required.  Otherwise the exception would propagate to the top of
+the  stack and would cause the whole function to abort with
+a PostgreSQL error, so that the
+operations table would not have any row
+inserted into it.  The subtransaction command does not
+trap errors, it only assures that all database operations executed
+inside its scope will be atomically committed or rolled back.  A
+rollback of the subtransaction block occurs on any kind of
+exception exit, not only ones caused by errors originating from
+database access.  A regular Tcl exception raised inside an
+explicit subtransaction block would also cause the subtransaction
+to be rolled back.
+   
+  
 

 PL/Tcl Configuration
diff --git a/src/pl/tcl/Makefile b/src/pl/tcl/Makefile
index 1096c4f..b6b6b19 100644
--- a/src/pl/tcl/Makefile
+++ b/src/pl/tcl/Makefile
@@ -28,7 +28,7 @@ DATA = pltcl.control pltcl--1.0.sql pltcl--unpackaged--1.0.sql \
pltclu.control pltclu--1.0.sql pltclu--unpackaged--1.0.sql
 
 REGRESS_OPTS = --dbname=$(PL_TESTDB) --load-extension=pltcl
-REGRESS = pltcl_setup pltcl_queries pltcl_start_proc pltcl_unicode
+REGRESS = pltcl_setup pltcl_queries pltcl_start_proc pltcl_unicode pltcl_subxact
 
 # Tcl on win32 ships with import libraries only for Microsoft Visual C++,
 # which are not compatible with mingw gcc. Therefore we need to build a
diff --git a/src/pl/tcl/expected/pltcl_subxact.out b/src/pl/tcl/expected/pltcl_subxact.out
new file mode 100644
index 000..5d984f1
--- /dev/null
+++ b/src/pl/tcl/expected/pltcl_subxact.out
@@ -0,0 +1,171 @@
+--
+-- Test explicit subtransactions
+--
+CREATE TABLE subtransaction_tbl (
+i integer
+);
+-- test subtransaction successfully commited
+CREATE FUNCTION subtransaction_ctx_success() RETURNS text
+AS $$
+	spi_exec "INSERT INTO subtransaction_tbl VALUES(1)"
+subtransaction {
+		spi_exec "INSERT INTO subtransaction_tbl VALUES(2)"
+	}
+$$ LANGUAGE pltcl;
+BEGIN;
+INSERT INTO subtransaction_tbl VALUES(0);
+SELECT 

Re: [HACKERS] Explicit subtransactions for PL/Tcl

[Pavel Stehule]

2017-03-09 7:48 GMT+01:00 Victor Wagner :

> On Wed, 8 Mar 2017 16:49:33 +0100
> Pavel Stehule  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  level necessary? Probably there will not be
> > any other similar command.
>
> Removed . 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


> With best regards, Victor
>
> --
> Victor Wagner 
>

Re: [HACKERS] Explicit subtransactions for PL/Tcl

[Victor Wagner]

On Wed, 8 Mar 2017 16:49:33 +0100
Pavel Stehule  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  level necessary? Probably there will not be
> any other similar command.

Removed . 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 diff --git a/doc/src/sgml/pltcl.sgml b/doc/src/sgml/pltcl.sgml
index ad216dd..87bc4ad 100644
--- a/doc/src/sgml/pltcl.sgml
+++ b/doc/src/sgml/pltcl.sgml
@@ -901,6 +901,79 @@ if {[catch { spi_exec $sql_command }]} {
  is a global variable.)
 

+   
+   Explicit Subtransactions
+  
+   Recovering from errors caused by database access as described in
+can lead to an undesirable
+   situation where some operations succeed before one of them fails,
+   and after recovering from that error the data is left in an
+   inconsistent state.  PL/Tcl offers a solution to this problem in
+   the form of explicit subtransactions.
+  
+   
+Consider a function that implements a transfer between two
+accounts:
+
+CREATE FUNCTION transfer_funds() RETURNS void AS $$
+if [catch {
+spi_exec "UPDATE accounts SET balance = balance - 100 WHERE account_name = 'joe'"
+spi_exec "UPDATE accounts SET balance = balance + 100 WHERE account_name = 'mary'"
+} errormsg] {
+set result [format "error transferring funds: %s" $errormsg ]
+} else {
+set result "funds transferred correctly"
+}
+set plan [spi_prepare "INSERT INTO operations (result) VALUES ($1)"]
+spi_execp -count 1 $plan, [list $result)
+$$ LANGUAGE pltclu;
+
+If the second UPDATE statement results in an
+exception being raised, this function will report the error, but
+the result of the first UPDATE will
+nevertheless be committed.  In other words, the funds will be
+withdrawn from Joe's account, but will not be transferred to
+Mary's account.
+   
+   
+To avoid such issues, you can wrap your
+spi_exec calls in an explicit
+subtransaction.  The PL/Tcl provides a
+commmand subtransaction to manage explicit
+	subtransactions.
+ Using explicit subtransactions
+we can rewrite our function as:
+
+CREATE FUNCTION transfer_funds2() RETURNS void AS $$
+if [catch {
+	subtransaction {
+spi_exec "UPDATE accounts SET balance = balance - 100 WHERE account_name = 'joe'"
+spi_exec "UPDATE accounts SET balance = balance + 100 WHERE account_name = 'mary'"
+	}
+	} errormsg] {
+		set result [format "error transferring funds: %s" $errormsg]
+	} else {
+   set result "funds transferred correctly"
+	}
+set plan  [spi_prepare "INSERT INTO operations (result) VALUES ($1)"]
+spi_execp $plan, [list $result]
+$$ LANGUAGE pltclu;
+
+Note that the use of catch is still
+required.  Otherwise the exception would propagate to the top of
+the  stack and would cause the whole function to abort with
+a PostgreSQL error, so that the
+operations table would not have any row
+inserted into it.  The subtransaction command does not
+trap errors, it only assures that all database operations executed
+inside its scope will be atomically committed or rolled back.  A
+rollback of the subtransaction block occurs on any kind of
+exception exit, not only ones caused by errors originating from
+database access.  A regular Tcl exception raised inside an
+explicit subtransaction block would also cause the subtransaction
+to be rolled back.
+   
+  
 

 PL/Tcl 

Re: [HACKERS] Explicit subtransactions for PL/Tcl

[Victor Wagner]

On Wed, 8 Mar 2017 16:49:33 +0100
Pavel Stehule  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  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 


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

Re: [HACKERS] Explicit subtransactions for PL/Tcl

[Pavel Stehule]

Hi

2017-01-08 18:57 GMT+01:00 Victor Wagner :

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

+<->BeginInternalSubTransaction(NULL);
+<->MemoryContextSwitchTo(TopTransactionContext);
+<->


Regards

Pavel



>
> --
>Victor Wagner 
>

Re: [HACKERS] Explicit subtransactions for PL/Tcl

[Tom Lane]

Victor Wagner  writes:
> 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.

Interesting.  Please add this to the 2017-03 commitfest list, to make
sure we don't lose track of it.

regards, tom lane


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

Re: [HACKERS] Explicit subtransactions for PL/Tcl

[Victor Wagner]

On Sun, 8 Jan 2017 20:57:50 +0300
Victor Wagner  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 
diff --git a/doc/src/sgml/pltcl.sgml b/doc/src/sgml/pltcl.sgml
index 8afaf4a..7a532b7 100644
--- a/doc/src/sgml/pltcl.sgml
+++ b/doc/src/sgml/pltcl.sgml
@@ -901,6 +901,85 @@ if {[catch { spi_exec $sql_command }]} {
  is a global variable.)
 

+   
+   Explicit Subtransactions
+  
+   Recovering from errors caused by database access as described in
+can lead to an undesirable
+   situation where some operations succeed before one of them fails,
+   and after recovering from that error the data is left in an
+   inconsistent state.  PL/Tcl offers a solution to this problem in
+   the form of explicit subtransactions.
+  
+
+  
+   Subtransaction command
+
+   
+Consider a function that implements a transfer between two
+accounts:
+
+CREATE FUNCTION transfer_funds() RETURNS void AS $$
+if [catch {
+spi_exec "UPDATE accounts SET balance = balance - 100 WHERE account_name = 'joe'"
+spi_exec "UPDATE accounts SET balance = balance + 100 WHERE account_name = 'mary'"
+} errormsg] {
+set result [format "error transferring funds: %s" $errormsg ]
+} else {
+set result "funds transferred correctly"
+}
+set plan [spi_prepare "INSERT INTO operations (result) VALUES ($1)"]
+spi_execp -count 1 $plan, [list $result)
+$$ LANGUAGE pltclu;
+
+If the second UPDATE statement results in an
+exception being raised, this function will report the error, but
+the result of the first UPDATE will
+nevertheless be committed.  In other words, the funds will be
+withdrawn from Joe's account, but will not be transferred to
+Mary's account.
+   
+
+   
+To avoid such issues, you can wrap your
+spi_exec calls in an explicit
+subtransaction.  The PL/Tcl provides a
+commmand subtransaction to manage explicit
+	subtransactions.
+ Using explicit subtransactions
+we can rewrite our function as:
+
+CREATE FUNCTION transfer_funds2() RETURNS void AS $$
+if [catch {
+	subtransaction {
+spi_exec "UPDATE accounts SET balance = balance - 100 WHERE account_name = 'joe'"
+spi_exec "UPDATE accounts SET balance = balance + 100 WHERE account_name = 'mary'"
+	}
+	} errormsg] {
+		set result [format "error transferring funds: %s" $errormsg]
+	} else {
+   set result "funds transferred correctly"
+	}
+set plan  [spi_prepare "INSERT INTO operations (result) VALUES ($1)"]
+spi_execp $plan, [list $result]
+$$ LANGUAGE pltclu;
+
+Note that the use of catch is still
+required.  Otherwise the exception would propagate to the top of
+the  stack and would cause the whole function to abort with
+a PostgreSQL error, so that the
+operations table would not have any row
+inserted into it.  The subtransaction command does not
+trap errors, it only assures that all database operations executed
+inside its scope will be atomically committed or rolled back.  A
+rollback of the subtransaction block occurs on any kind of
+exception exit, not only ones caused by errors originating from
+database access.  A regular Tcl exception raised inside an
+explicit subtransaction block would also cause the subtransaction
+to be rolled back.
+   
+  
+  
 

Modules and the unknown Command
diff --git a/src/pl/tcl/Makefile b/src/pl/tcl/Makefile
index 25082ec..614385d 100644
--- a/src/pl/tcl/Makefile
+++ b/src/pl/tcl/Makefile
@@ -28,7 +28,7 @@ DATA = pltcl.control pltcl--1.0.sql pltcl--unpackaged--1.0.sql \
pltclu.control pltclu--1.0.sql pltclu--unpackaged--1.0.sql
 
 REGRESS_OPTS = --dbname=$(PL_TESTDB) --load-extension=pltcl
-REGRESS = pltcl_setup pltcl_queries pltcl_unicode
+REGRESS = pltcl_setup pltcl_queries pltcl_unicode pltcl_subxact
 
 # Tcl on win32 ships with import libraries only for Microsoft Visual C++,
 # which are not compatible with mingw gcc. Therefore we need to build a
diff --git a/src/pl/tcl/expected/pltcl_subxact.out b/src/pl/tcl/expected/pltcl_subxact.out
new file mode 100644
index 000..17b9d90
--- /dev/null
+++ b/src/pl/tcl/expected/pltcl_subxact.out
@@ -0,0 +1,145 @@
+--
+-- Test explicit subtransactions
+--
+-- Test table to see if transactions get properly rolled back
+CREATE TABLE subtransaction_tbl (
+i integer
+);
+CREATE FUNCTION subtransaction_ctx_test(what_error text = NULL) RETURNS text
+AS $$
+subtransaction {
+spi_exec "INSERT INTO subtransaction_tbl VALUES (1)"
+spi_exec "INSERT INTO subtransaction_tbl VALUES (2)"
+if {$1 == "SPI"} {
+spi_exec "INSERT INTO