Re: [HACKERS] Transaction control in procedures

[Peter Eisentraut]

On 1/16/18 15:24, Andrew Dunstan wrote:
> Looks good. Marking ready for committer.

committed

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Transaction control in procedures

[Simon Riggs]

On 16 January 2018 at 20:24, Andrew Dunstan
 wrote:

> Looks good. Marking ready for committer.

Few questions/points for the docs.

Docs say: "A new transaction is started automatically after a
transaction is ended using these commands"
Presumably this would have exactly the same isolation level and other
transaction characteristics?
(Is it somehow possible to vary that. OK if not, no problem)

The error "cannot commit while a subtransaction is active"
is commented as intending to prevent COMMIT/ROLLBACK inside an EXCEPTION block.
That makes sense. It seems it will also prevent SAVEPOINTs, though
that seems not to be intended.
The two cases are dissimilar and it would be possible to block the
former but allow the latter.

It's not documented or tested that SET LOCAL would work or not work.
Does it work?

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Transaction control in procedures

[Simon Riggs]

On 6 December 2017 at 22:34, Merlin Moncure  wrote:
> On Wed, Dec 6, 2017 at 8:41 AM, Peter Eisentraut
>  wrote:
>> On 12/5/17 13:33, Robert Haas wrote:
>>> On Tue, Dec 5, 2017 at 1:25 PM, Peter Eisentraut
>>>  wrote:
 I think ROLLBACK in a cursor loop might not make sense, because the
 cursor query itself could have side effects, so a rollback would have to
 roll back the entire loop.  That might need more refined analysis before
 it could be allowed.
>>>
>>> COMMIT really has the same problem; if the cursor query has side
>>> effects, you can't commit those side effects piecemeal as the loop
>>> executed and have things behave sanely.
>>
>> The first COMMIT inside the loop would commit the cursor query.  This
>> isn't all that different from what you'd get now if you coded this
>> manually using holdable cursors or just plain client code.  Clearly, you
>> can create a mess if the loop body interacts with the loop expression,
>> but that's already the case.
>>
>> But if you coded something like this yourself now and ran a ROLLBACK
>> inside the loop, the holdable cursor would disappear (unless previously
>> committed), so you couldn't proceed with the loop.
>>
>> The SQL standard for persistent stored modules explicitly prohibits
>> COMMIT and ROLLBACK in cursor loop bodies.  But I think people will
>> eventually want it.
>
> The may want it, but silently promoting all cursors to held ones is
> not the way to give it to them, unless we narrow it down the the
> 'for-loop derived cursor' only.

I don't think we should do that automatically for all cursors, but it
seems clear that we would want that iff the loop contains COMMIT or
ROLLBACK.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Transaction control in procedures

[Andrew Dunstan]

On 01/16/2018 10:16 AM, Peter Eisentraut wrote:
> On 1/15/18 12:57, Andrew Dunstan wrote:
>> This confused me slightly:
>>
>> +    Transactions cannot be ended inside loops through query results
>> or inside
>> +    blocks with exception handlers.
>>
>> I suggest: "A transaction cannot be ended inside a loop over query
>> results, nor inside a block with exception handlers."
> fixed
>
>> The patch has bitrotted slightly in src/backend/commands/portalcmds.c
> merged
>
>> The plperl expected file needs updating. Also, why does spi_commit() in
>> a loop result in an error message but not spi_rollback()?
> This is all changed now after the patch for portal pinning in PL/Perl
> and PL/Python has been committed.  The attached patch behaves better.
>


Looks good. Marking ready for committer.

cheers

andrew

-- 
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Transaction control in procedures

[Peter Eisentraut]

On 1/15/18 12:57, Andrew Dunstan wrote:
> This confused me slightly:
> 
> +    Transactions cannot be ended inside loops through query results
> or inside
> +    blocks with exception handlers.
> 
> I suggest: "A transaction cannot be ended inside a loop over query
> results, nor inside a block with exception handlers."

fixed

> The patch has bitrotted slightly in src/backend/commands/portalcmds.c

merged

> The plperl expected file needs updating. Also, why does spi_commit() in
> a loop result in an error message but not spi_rollback()?

This is all changed now after the patch for portal pinning in PL/Perl
and PL/Python has been committed.  The attached patch behaves better.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 84f35266cc63de1d433d8a5e3549a0e28cb4c1fc Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Tue, 16 Jan 2018 10:12:44 -0500
Subject: [PATCH v7] Transaction control in PL procedures

In each of the supplied procedural languages (PL/pgSQL, PL/Perl,
PL/Python, PL/Tcl), add language-specific commit and rollback
functions/commands to control transactions in procedures in that
language.  Add similar underlying functions to SPI.  Some additional
cleanup so that transaction commit or abort doesn't blow away data
structures still used by the procedure call.  Add execution context
tracking to CALL and DO statements so that transaction control commands
can only be issued in top-level procedure and block calls, not function
calls or other procedure or block calls.

- SPI

Add a new function SPI_connect_ext() that is like SPI_connect() but
allows passing option flags.  The only option flag right now is
SPI_OPT_NONATOMIC.  A nonatomic SPI connection can execute transaction
control commands, otherwise it's not allowed.  This is meant to be
passed down from CALL and DO statements which themselves know in which
context they are called.  A nonatomic SPI connection uses different
memory management.  A normal SPI connection allocates its memory in
TopTransactionContext.  For nonatomic connections we use PortalContext
instead.  As the comment in SPI_connect_ext() (previously SPI_connect())
indicates, one could potentially use PortalContext in all cases, but it
seems safest to leave the existing uses alone, because this stuff is
complicated enough already.

SPI also gets new functions SPI_start_transaction(), SPI_commit(), and
SPI_rollback(), which can be used by PLs to implement their transaction
control logic.

- portalmem.c

Some adjustments were made in the code that cleans up portals at
transaction abort.  The portal code could already handle a command
*committing* a transaction and continuing (e.g., VACUUM), but it was not
quite prepared for a command *aborting* a transaction and continuing.

In AtAbort_Portals(), remove the code that marks an active portal as
failed.  As the comment there already predicted, this doesn't work if
the running command wants to keep running after transaction abort.  And
it's actually not necessary, because pquery.c is careful to run all
portal code in a PG_TRY block and explicitly runs MarkPortalFailed() if
there is an exception.  So the code in AtAbort_Portals() is never used
anyway.

In AtAbort_Portals() and AtCleanup_Portals(), we need to be careful not
to clean up active portals too much.  This mirrors similar code in
PreCommit_Portals().

- PL/Perl

Gets new functions spi_commit() and spi_rollback()

- PL/pgSQL

Gets new commands COMMIT and ROLLBACK.

Update the PL/SQL porting example in the documentation to reflect that
transactions are now possible in procedures.

- PL/Python

Gets new functions plpy.commit and plpy.rollback.

- PL/Tcl

Gets new commands commit and rollback.
---
 doc/src/sgml/plperl.sgml   |  49 +
 doc/src/sgml/plpgsql.sgml  |  91 
 doc/src/sgml/plpython.sgml |  39 
 doc/src/sgml/pltcl.sgml|  41 
 doc/src/sgml/ref/call.sgml |   7 +
 doc/src/sgml/ref/create_procedure.sgml |   7 +
 doc/src/sgml/ref/do.sgml   |   7 +
 doc/src/sgml/spi.sgml  | 177 +++
 src/backend/commands/functioncmds.c|  45 +++-
 src/backend/executor/spi.c |  99 -
 src/backend/tcop/utility.c |   6 +-
 src/backend/utils/mmgr/portalmem.c |  49 +++--
 src/include/commands/defrem.h  |   4 +-
 src/include/executor/spi.h |   7 +
 src/include/executor/spi_priv.h|   4 +
 src/include/nodes/nodes.h  |   3 +-
 src/include/nodes/parsenodes.h |   7 +
 src/include/utils/portal.h |   1 +
 src/pl/plperl/GNUmakefile  |   2 

Re: [HACKERS] Transaction control in procedures

[Andrew Dunstan]

On 01/05/2018 04:30 PM, Peter Eisentraut wrote:
> A merge conflict has arisen, so for simplicity, here is an updated patch.
>
> On 12/20/17 10:08, Peter Eisentraut wrote:
>> Updated patch attached.
>>
>> I have addressed the most recent review comments I believe.
>>
>> The question about what happens to cursor loops in PL/Perl and PL/Python
>> would be addressed by the separate thread "portal pinning".  The test
>> cases in this patch are currently marked by FIXMEs.
>>
>> I have changed the SPI API a bit.  I got rid of SPI_set_nonatomic() and
>> instead introduced SPI_connect_ext() that you can pass flags to.  The
>> advantage of that is that in the normal case we can continue to use the
>> existing memory contexts, so nothing changes for existing uses, which
>> seems desirable.  (This also appears to address some sporadic test
>> failures in PL/Perl.)
>>
>> I have also cleaned up the changes in portalmem.c further, so the
>> changes are now even smaller.
>>
>> The commit message in this patch contains more details about some of
>> these changes.


Generally looks good.

This confused me slightly:

+    Transactions cannot be ended inside loops through query results
or inside
+    blocks with exception handlers.

I suggest: "A transaction cannot be ended inside a loop over query
results, nor inside a block with exception handlers."

The patch has bitrotted slightly in src/backend/commands/portalcmds.c

The plperl expected file needs updating. Also, why does spi_commit() in
a loop result in an error message but not spi_rollback()?


cheers

andrew



-- 
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Transaction control in procedures

[Peter Eisentraut]

A merge conflict has arisen, so for simplicity, here is an updated patch.

On 12/20/17 10:08, Peter Eisentraut wrote:
> Updated patch attached.
> 
> I have addressed the most recent review comments I believe.
> 
> The question about what happens to cursor loops in PL/Perl and PL/Python
> would be addressed by the separate thread "portal pinning".  The test
> cases in this patch are currently marked by FIXMEs.
> 
> I have changed the SPI API a bit.  I got rid of SPI_set_nonatomic() and
> instead introduced SPI_connect_ext() that you can pass flags to.  The
> advantage of that is that in the normal case we can continue to use the
> existing memory contexts, so nothing changes for existing uses, which
> seems desirable.  (This also appears to address some sporadic test
> failures in PL/Perl.)
> 
> I have also cleaned up the changes in portalmem.c further, so the
> changes are now even smaller.
> 
> The commit message in this patch contains more details about some of
> these changes.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 86db8254bcd93585f1036b755a54ae1608ed092f Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Fri, 5 Jan 2018 16:27:53 -0500
Subject: [PATCH v6] Transaction control in PL procedures

In each of the supplied procedural languages (PL/pgSQL, PL/Perl,
PL/Python, PL/Tcl), add language-specific commit and rollback
functions/commands to control transactions in procedures in that
language.  Add similar underlying functions to SPI.  Some additional
cleanup so that transaction commit or abort doesn't blow away data
structures still used by the procedure call.  Add execution context
tracking to CALL and DO statements so that transaction control commands
can only be issued in top-level procedure and block calls, not function
calls or other procedure or block calls.

- SPI

Add a new function SPI_connect_ext() that is like SPI_connect() but
allows passing option flags.  The only option flag right now is
SPI_OPT_NONATOMIC.  A nonatomic SPI connection can execute transaction
control commands, otherwise it's not allowed.  This is meant to be
passed down from CALL and DO statements which themselves know in which
context they are called.  A nonatomic SPI connection uses different
memory management.  A normal SPI connection allocates its memory in
TopTransactionContext.  For nonatomic connections we use PortalContext
instead.  As the comment in SPI_connect_ext() (previously SPI_connect())
indicates, one could potentially use PortalContext in all cases, but it
seems safest to leave the existing uses alone, because this stuff is
complicated enough already.

SPI also gets new functions SPI_start_transaction(), SPI_commit(), and
SPI_rollback(), which can be used by PLs to implement their transaction
control logic.

- portalmem.c

Some adjustments were made in the code that cleans up portals at
transaction abort.  The portal code could already handle a command
*committing* a transaction and continuing (e.g., VACUUM), but it was not
quite prepared for a command *aborting* a transaction and continuing.

In AtAbort_Portals(), remove the code that marks an active portal as
failed.  As the comment there already predicted, this doesn't work if
the running command wants to keep running after transaction abort.  And
it's actually not necessary, because pquery.c is careful to run all
portal code in a PG_TRY block and explicitly runs MarkPortalFailed() if
there is an exception.  So the code in AtAbort_Portals() is never used
anyway.

In AtAbort_Portals() and AtCleanup_Portals(), we need to be careful not
to clean up active portals too much.  This mirrors similar code in
PreCommit_Portals().

- PL/Perl

Gets new functions spi_commit() and spi_rollback()

- PL/pgSQL

Gets new commands COMMIT and ROLLBACK.

Update the PL/SQL porting example in the documentation to reflect that
transactions are now possible in procedures.

- PL/Python

Gets new functions plpy.commit and plpy.rollback.

- PL/Tcl

Gets new commands commit and rollback.
---
 doc/src/sgml/plperl.sgml   |  49 +
 doc/src/sgml/plpgsql.sgml  |  91 
 doc/src/sgml/plpython.sgml |  39 
 doc/src/sgml/pltcl.sgml|  41 
 doc/src/sgml/ref/call.sgml |   7 +
 doc/src/sgml/ref/create_procedure.sgml |   7 +
 doc/src/sgml/ref/do.sgml   |   7 +
 doc/src/sgml/spi.sgml  | 177 +++
 src/backend/commands/functioncmds.c|  45 +++-
 src/backend/executor/spi.c |  99 -
 src/backend/tcop/utility.c |   6 +-
 src/backend/utils/mmgr/portalmem.c |  49 +++--
 src/include/commands/defrem.h  |   4 +-
 src/include/executor/spi.h |   7 +
 

Re: [HACKERS] Transaction control in procedures

[Peter Eisentraut]

Updated patch attached.

I have addressed the most recent review comments I believe.

The question about what happens to cursor loops in PL/Perl and PL/Python
would be addressed by the separate thread "portal pinning".  The test
cases in this patch are currently marked by FIXMEs.

I have changed the SPI API a bit.  I got rid of SPI_set_nonatomic() and
instead introduced SPI_connect_ext() that you can pass flags to.  The
advantage of that is that in the normal case we can continue to use the
existing memory contexts, so nothing changes for existing uses, which
seems desirable.  (This also appears to address some sporadic test
failures in PL/Perl.)

I have also cleaned up the changes in portalmem.c further, so the
changes are now even smaller.

The commit message in this patch contains more details about some of
these changes.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 984d2974311ce6d8760ec3d59af66363cc53fc6d Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Wed, 20 Dec 2017 09:25:45 -0500
Subject: [PATCH v5] Transaction control in PL procedures

In each of the supplied procedural languages (PL/pgSQL, PL/Perl,
PL/Python, PL/Tcl), add language-specific commit and rollback
functions/commands to control transactions in procedures in that
language.  Add similar underlying functions to SPI.  Some additional
cleanup so that transaction commit or abort doesn't blow away data
structures still used by the procedure call.  Add execution context
tracking to CALL and DO statements so that transaction control commands
can only be issued in top-level procedure and block calls, not function
calls or other procedure or block calls.

- SPI

Add a new function SPI_connect_ext() that is like SPI_connect() but
allows passing option flags.  The only option flag right now is
SPI_OPT_NONATOMIC.  A nonatomic SPI connection can execute transaction
control commands, otherwise it's not allowed.  This is meant to be
passed down from CALL and DO statements which themselves know in which
context they are called.  A nonatomic SPI connection uses different
memory management.  A normal SPI connection allocates its memory in
TopTransactionContext.  For nonatomic connections we use PortalContext
instead.  As the comment in SPI_connect_ext() (previously SPI_connect())
indicates, one could potentially use PortalContext in all cases, but it
seems safest to leave the existing uses alone, because this stuff is
complicated enough already.

SPI also gets new functions SPI_start_transaction(), SPI_commit(), and
SPI_rollback(), which can be used by PLs to implement their transaction
control logic.

- portalmem.c

Some adjustments were made in the code that cleans up portals at
transaction abort.  The portal code could already handle a command
*committing* a transaction and continuing (e.g., VACUUM), but it was not
quite prepared for a command *aborting* a transaction and continuing.

In AtAbort_Portals(), remove the code that marks an active portal as
failed.  As the comment there already predicted, this doesn't work if
the running command wants to keep running after transaction abort.  And
it's actually not necessary, because pquery.c is careful to run all
portal code in a PG_TRY block and explicitly runs MarkPortalFailed() if
there is an exception.  So the code in AtAbort_Portals() is never used
anyway.

In AtAbort_Portals() and AtCleanup_Portals(), we need to be careful not
to clean up active portals too much.  This mirrors similar code in
PreCommit_Portals().

- PL/Perl

Gets new functions spi_commit() and spi_rollback()

- PL/pgSQL

Gets new commands COMMIT and ROLLBACK.

Update the PL/SQL porting example in the documentation to reflect that
transactions are now possible in procedures.

- PL/Python

Gets new functions plpy.commit and plpy.rollback.

- PL/Tcl

Gets new commands commit and rollback.
---
 doc/src/sgml/plperl.sgml   |  49 +
 doc/src/sgml/plpgsql.sgml  |  91 
 doc/src/sgml/plpython.sgml |  39 
 doc/src/sgml/pltcl.sgml|  41 
 doc/src/sgml/ref/call.sgml |   7 +
 doc/src/sgml/ref/create_procedure.sgml |   7 +
 doc/src/sgml/ref/do.sgml   |   7 +
 doc/src/sgml/spi.sgml  | 177 +++
 src/backend/commands/functioncmds.c|  45 +++-
 src/backend/executor/spi.c |  99 -
 src/backend/tcop/utility.c |   6 +-
 src/backend/utils/mmgr/portalmem.c |  49 +++--
 src/include/commands/defrem.h  |   4 +-
 src/include/executor/spi.h |   7 +
 src/include/executor/spi_priv.h|   4 +
 src/include/nodes/nodes.h  |   3 +-
 src/include/nodes/parsenodes.h  

Re: [HACKERS] Transaction control in procedures

[Peter Eisentraut]

On 12/7/17 18:47, Andrew Dunstan wrote:
> Referring to anonymous blocks might be a bit mystifying for some
> readers, unless we note that they are invoked via DO.

Added parenthetical comments.

> I think this sentence should probably be worded a bit differently:
> 
> (And of course, BEGIN and END have different meanings in PL/pgSQL.)
> 
> Perhaps "Note that" instead of "And of course,".

fixed

> Why aren't the SPI functions that error out or return 0 just void
> instead of returning an int?

Tried to align then with existing functions, but I agree it seems weird.
 Changed to return void.

> The docs say SPI_set_non_atomic() returns 0 on success, but the code
> says otherwise.

Fixed the documentation.

> This sentence in the comment in SPI_Commit() is slightly odd:
> 
> This restriction is particular to PLs implemented on top of SPI.
> 
> Perhaps "required by" rather than "particular to" would make it read better.

fixed

> The empty free_commit() and free_rollback() functions in pl_funcs.c look
> slightly odd. I realize that the compiler should optimize the calls
> away, but it seems an odd style.

That's how the existing code for other statements looks as well.

> One other thing I wondered about was what if a PL function (say plperl)
> used SPI to open an explicit cursor and then looped over it? If there
> were a commit or rollback inside that loop we wouldn't have the same
> protections we have in plpgsql, ISTM. I haven't tried this yet, so I'm
> just speculating about what might happen.

Good point.  I added test cases similar to the plpgsql tests to the
other three languages, which not-so-amusingly gave three different
outcomes.  In PL/Perl in particular, the commit clears away the portal,
and the next call to spi_fetchrow() will then not find the cursor and
just return undefined.  So that is not so nice.  I'm thinking about
extending the portal pinning mechanism to the other languages as well,
which seems mildly useful independent of transaction management.  I will
propose a patch for that soon.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Transaction control in procedures

[Andrew Dunstan]

On 12/06/2017 09:41 AM, Peter Eisentraut wrote:
> On 12/5/17 13:33, Robert Haas wrote:
>> On Tue, Dec 5, 2017 at 1:25 PM, Peter Eisentraut
>>  wrote:
>>> I think ROLLBACK in a cursor loop might not make sense, because the
>>> cursor query itself could have side effects, so a rollback would have to
>>> roll back the entire loop.  That might need more refined analysis before
>>> it could be allowed.
>> COMMIT really has the same problem; if the cursor query has side
>> effects, you can't commit those side effects piecemeal as the loop
>> executed and have things behave sanely.
> The first COMMIT inside the loop would commit the cursor query.  This
> isn't all that different from what you'd get now if you coded this
> manually using holdable cursors or just plain client code.  Clearly, you
> can create a mess if the loop body interacts with the loop expression,
> but that's already the case.
>
> But if you coded something like this yourself now and ran a ROLLBACK
> inside the loop, the holdable cursor would disappear (unless previously
> committed), so you couldn't proceed with the loop.
>
> The SQL standard for persistent stored modules explicitly prohibits
> COMMIT and ROLLBACK in cursor loop bodies.  But I think people will
> eventually want it.
>
 - COMMIT or ROLLBACK inside a procedure with a SET clause attached,
>>> That also needs to be prohibited because of the way the GUC nesting
>>> currently works.  It's probably possible to fix it, but it would be a
>>> separate effort.
>>>
 and/or while SET LOCAL is in effect either at the inner or outer
 level.
>>> That seems to work fine.
>> These two are related -- if you don't permit anything that makes
>> temporary changes to GUCs at all, like SET clauses attached to
>> functions, then SET LOCAL won't cause any problems.  The problem is if
>> you do a transaction operation when something set locally is in the
>> stack of values, but not at the top.
> Yes, that's exactly the problem.  So right now I'm just preventing the
> problematic scenario.  So fix that, one would possibly have to replace
> the stack by something not quite a stack.
>
>
> New patch attached.
>



In general this looks good. However, I'm getting runtime errors in
plperl_elog.c on two different Linux platforms (Fedora 25, Ubuntu
16.04). There seems to be something funky going on. And we do need to
work out why the commented out plperl test isn't working.


Detailed comments:

Referring to anonymous blocks might be a bit mystifying for some
readers, unless we note that they are invoked via DO.

I think this sentence should probably be worded a bit differently:

(And of course, BEGIN and END have different meanings in PL/pgSQL.)

Perhaps "Note that" instead of "And of course,".

Why aren't the SPI functions that error out or return 0 just void
instead of returning an int?

The docs say SPI_set_non_atomic() returns 0 on success, but the code
says otherwise.

This sentence in the comment in SPI_Commit() is slightly odd:

This restriction is particular to PLs implemented on top of SPI.

Perhaps "required by" rather than "particular to" would make it read better.

The empty free_commit() and free_rollback() functions in pl_funcs.c look
slightly odd. I realize that the compiler should optimize the calls
away, but it seems an odd style.


One other thing I wondered about was what if a PL function (say plperl)
used SPI to open an explicit cursor and then looped over it? If there
were a commit or rollback inside that loop we wouldn't have the same
protections we have in plpgsql, ISTM. I haven't tried this yet, so I'm
just speculating about what might happen.



cheers

andrew


-- 
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Transaction control in procedures

[Merlin Moncure]

On Wed, Dec 6, 2017 at 8:41 AM, Peter Eisentraut
 wrote:
> On 12/5/17 13:33, Robert Haas wrote:
>> On Tue, Dec 5, 2017 at 1:25 PM, Peter Eisentraut
>>  wrote:
>>> I think ROLLBACK in a cursor loop might not make sense, because the
>>> cursor query itself could have side effects, so a rollback would have to
>>> roll back the entire loop.  That might need more refined analysis before
>>> it could be allowed.
>>
>> COMMIT really has the same problem; if the cursor query has side
>> effects, you can't commit those side effects piecemeal as the loop
>> executed and have things behave sanely.
>
> The first COMMIT inside the loop would commit the cursor query.  This
> isn't all that different from what you'd get now if you coded this
> manually using holdable cursors or just plain client code.  Clearly, you
> can create a mess if the loop body interacts with the loop expression,
> but that's already the case.
>
> But if you coded something like this yourself now and ran a ROLLBACK
> inside the loop, the holdable cursor would disappear (unless previously
> committed), so you couldn't proceed with the loop.
>
> The SQL standard for persistent stored modules explicitly prohibits
> COMMIT and ROLLBACK in cursor loop bodies.  But I think people will
> eventually want it.

The may want it, but silently promoting all cursors to held ones is
not the way to give it to them, unless we narrow it down the the
'for-loop derived cursor' only.  Even then we should consider syntax
decoration:

FOR x IN SELECT  WITH HOLD
LOOP

END LOOP;

merlin

Re: [HACKERS] Transaction control in procedures

[Peter Eisentraut]

On 12/5/17 13:33, Robert Haas wrote:
> On Tue, Dec 5, 2017 at 1:25 PM, Peter Eisentraut
>  wrote:
>> I think ROLLBACK in a cursor loop might not make sense, because the
>> cursor query itself could have side effects, so a rollback would have to
>> roll back the entire loop.  That might need more refined analysis before
>> it could be allowed.
> 
> COMMIT really has the same problem; if the cursor query has side
> effects, you can't commit those side effects piecemeal as the loop
> executed and have things behave sanely.

The first COMMIT inside the loop would commit the cursor query.  This
isn't all that different from what you'd get now if you coded this
manually using holdable cursors or just plain client code.  Clearly, you
can create a mess if the loop body interacts with the loop expression,
but that's already the case.

But if you coded something like this yourself now and ran a ROLLBACK
inside the loop, the holdable cursor would disappear (unless previously
committed), so you couldn't proceed with the loop.

The SQL standard for persistent stored modules explicitly prohibits
COMMIT and ROLLBACK in cursor loop bodies.  But I think people will
eventually want it.

>>> - COMMIT or ROLLBACK inside a procedure with a SET clause attached,
>>
>> That also needs to be prohibited because of the way the GUC nesting
>> currently works.  It's probably possible to fix it, but it would be a
>> separate effort.
>>
>>> and/or while SET LOCAL is in effect either at the inner or outer
>>> level.
>>
>> That seems to work fine.
> 
> These two are related -- if you don't permit anything that makes
> temporary changes to GUCs at all, like SET clauses attached to
> functions, then SET LOCAL won't cause any problems.  The problem is if
> you do a transaction operation when something set locally is in the
> stack of values, but not at the top.

Yes, that's exactly the problem.  So right now I'm just preventing the
problematic scenario.  So fix that, one would possibly have to replace
the stack by something not quite a stack.


New patch attached.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 18aaf292fbb22647e09c38cc21b56ff98643e518 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Wed, 6 Dec 2017 09:29:25 -0500
Subject: [PATCH v4] Transaction control in PL procedures

In each of the supplied procedural languages (PL/pgSQL, PL/Perl,
PL/Python, PL/Tcl), add language-specific commit and rollback
functions/commands to control transactions in procedures in that
language.  Add similar underlying functions to SPI.  Some additional
cleanup so that transaction commit or abort doesn't blow away data
structures still used by the procedure call.  Add execution context
tracking to CALL and DO statements so that transaction control commands
can only be issued in top-level procedure and block calls, not function
calls or other procedure or block calls.
---
 doc/src/sgml/plperl.sgml  |  49 +
 doc/src/sgml/plpgsql.sgml |  91 
 doc/src/sgml/plpython.sgml|  38 
 doc/src/sgml/pltcl.sgml   |  41 
 doc/src/sgml/ref/call.sgml|   7 +
 doc/src/sgml/ref/create_procedure.sgml|   7 +
 doc/src/sgml/ref/do.sgml  |   7 +
 doc/src/sgml/spi.sgml | 219 
 src/backend/commands/functioncmds.c   |  45 +++-
 src/backend/executor/spi.c| 112 --
 src/backend/tcop/utility.c|   6 +-
 src/backend/utils/mmgr/portalmem.c|  58 +++---
 src/include/commands/defrem.h |   4 +-
 src/include/executor/spi.h|   5 +
 src/include/executor/spi_priv.h   |   4 +
 src/include/nodes/nodes.h |   3 +-
 src/include/nodes/parsenodes.h|   7 +
 src/include/utils/portal.h|   1 +
 src/pl/plperl/GNUmakefile |   2 +-
 src/pl/plperl/SPI.xs  |  12 ++
 src/pl/plperl/expected/plperl_transaction.out |  94 +
 src/pl/plperl/plperl.c|   6 +
 src/pl/plperl/sql/plperl_transaction.sql  |  88 
 src/pl/plpgsql/src/pl_exec.c  |  66 +-
 src/pl/plpgsql/src/pl_funcs.c |  44 
 src/pl/plpgsql/src/pl_gram.y  |  34 +++
 src/pl/plpgsql/src/pl_handler.c   |   8 +
 src/pl/plpgsql/src/pl_scanner.c   |   2 +
 src/pl/plpgsql/src/plpgsql.h  |  22 +-
 src/pl/plpython/Makefile  |   1 +
 src/pl/plpython/expected/plpython_test.out|   4 +-
 src/pl/plpython/expected/plpython_transaction.out | 104 

Re: [HACKERS] Transaction control in procedures

[Robert Haas]

On Fri, Dec 1, 2017 at 2:48 PM, Peter Eisentraut
 wrote:
> Here is a new patch, now on top of master.  The main changes are that a
> lot of documentation has been added.

This feature doesn't have many tests.  I think it should have a lot
more of them.  It's tinkering with the transaction control machinery
of the system in a fairly fundamental way, and that could break
things.

I suggest, in particular, testing how it interactions with resources
such as cursors and prepared statements.  For example, what happens if
you commit or roll back inside a cursor-for loop (given that the
cursor is not holdable)?There are several kinds of cursor loops in
PostgreSQL, plus there are cursors, prepared statements, and portals
that can be created using SQL commands or protocol messages.  I
suspect that there are quite a few tricky interactions there.

Other things to think about:

- COMMIT or ROLLBACK inside a PLpgsql block with an attached EXCEPTION
block, or when an SQL SAVEPOINT has been established previously.

- COMMIT or ROLLBACK inside a procedure with a SET clause attached,
and/or while SET LOCAL is in effect either at the inner or outer
level.

- COMMIT or ROLLBACK with open large objects.

- COMMIT inside a procedure fails because of a serialization failure,
deferred constraint, etc.

In some cases, there are not only questions of correctness (it
shouldn't crash/give wrong answers) but also definitional questions
(what exactly should happen in that case, anyway?).

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [HACKERS] Transaction control in procedures

[Peter Eisentraut]

On 11/14/17 18:38, Peter Eisentraut wrote:
> On 10/31/17 15:38, Peter Eisentraut wrote:
>> Here is a patch that implements transaction control in PL/Python
>> procedures.  (This patch goes on top of "SQL procedures" patch v1.)
> 
> Here is an updated patch, now on top of "SQL procedures" v2.

Here is a new patch, now on top of master.  The main changes are that a
lot of documentation has been added.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 1a5e5fde2c0da663cc010b3e19418d0b2141304b Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Fri, 1 Dec 2017 14:40:29 -0500
Subject: [PATCH v3] Transaction control in PL procedures

In each of the supplied procedural languages (PL/pgSQL, PL/Perl,
PL/Python, PL/Tcl), add language-specific commit and rollback
functions/commands to control transactions in procedures in that
language.  Add similar underlying functions to SPI.  Some additional
cleanup so that transaction commit or abort doesn't blow away data
structures still used by the procedure call.  Add execution context
tracking to CALL and DO statements so that transaction control commands
can only be issued in top-level procedure and block calls, not function
calls or other procedure or block calls.
---
 doc/src/sgml/plperl.sgml  |  49 +
 doc/src/sgml/plpgsql.sgml |  86 +
 doc/src/sgml/plpython.sgml|  34 
 doc/src/sgml/pltcl.sgml   |  37 
 doc/src/sgml/spi.sgml | 219 ++
 src/backend/commands/functioncmds.c   |  30 ++-
 src/backend/executor/spi.c|  92 +++--
 src/backend/tcop/utility.c|   4 +-
 src/backend/utils/mmgr/portalmem.c|  39 ++--
 src/include/commands/defrem.h |   4 +-
 src/include/executor/spi.h|   5 +
 src/include/executor/spi_priv.h   |   4 +
 src/include/nodes/nodes.h |   3 +-
 src/include/nodes/parsenodes.h|   7 +
 src/pl/plperl/GNUmakefile |   2 +-
 src/pl/plperl/SPI.xs  |  12 ++
 src/pl/plperl/expected/plperl_transaction.out |  94 ++
 src/pl/plperl/plperl.c|   6 +
 src/pl/plperl/sql/plperl_transaction.sql  |  88 +
 src/pl/plpgsql/src/pl_exec.c  |  48 -
 src/pl/plpgsql/src/pl_funcs.c |  44 +
 src/pl/plpgsql/src/pl_gram.y  |  34 
 src/pl/plpgsql/src/pl_handler.c   |   8 +
 src/pl/plpgsql/src/pl_scanner.c   |   2 +
 src/pl/plpgsql/src/plpgsql.h  |  22 ++-
 src/pl/plpython/Makefile  |   1 +
 src/pl/plpython/expected/plpython_test.out|   4 +-
 src/pl/plpython/expected/plpython_transaction.out |  96 ++
 src/pl/plpython/plpy_main.c   |   7 +-
 src/pl/plpython/plpy_plpymodule.c |  28 +++
 src/pl/plpython/sql/plpython_transaction.sql  |  80 
 src/pl/tcl/Makefile   |   2 +-
 src/pl/tcl/expected/pltcl_transaction.out |  63 +++
 src/pl/tcl/pltcl.c|  44 +
 src/pl/tcl/sql/pltcl_transaction.sql  |  60 ++
 src/test/regress/expected/plpgsql.out | 110 +++
 src/test/regress/sql/plpgsql.sql  |  95 ++
 37 files changed, 1461 insertions(+), 102 deletions(-)
 create mode 100644 src/pl/plperl/expected/plperl_transaction.out
 create mode 100644 src/pl/plperl/sql/plperl_transaction.sql
 create mode 100644 src/pl/plpython/expected/plpython_transaction.out
 create mode 100644 src/pl/plpython/sql/plpython_transaction.sql
 create mode 100644 src/pl/tcl/expected/pltcl_transaction.out
 create mode 100644 src/pl/tcl/sql/pltcl_transaction.sql

diff --git a/doc/src/sgml/plperl.sgml b/doc/src/sgml/plperl.sgml
index 100162dead..82ddf26606 100644
--- a/doc/src/sgml/plperl.sgml
+++ b/doc/src/sgml/plperl.sgml
@@ -661,6 +661,55 @@ Database Access from PL/Perl
 
 
 
+
+
+ 
+  spi_commit()
+  
+   spi_commit
+   in PL/Perl
+ 
+ 
+ 
+  spi_rollback()
+  
+   spi_rollback
+   in PL/Perl
+  
+ 
+ 
+  
+   Commit or roll back the current transaction.  This can only be called
+   in a procedure or anonymous code block called from the top level.
+   (Note that it is not possible to run the SQL
+   commands COMMIT or ROLLBACK
+   via spi_exec_query or similar.  It has to be done
+   using these functions.)  After a transaction is ended, a new
+   transaction is automatically started, so there is no separate function
+   for that.
+  
+

Re: [HACKERS] Transaction control in procedures

[Peter Eisentraut]

On 10/31/17 15:38, Peter Eisentraut wrote:
> 2) SPI needs some work.  It thinks that it can clean everything away at
> transaction end.  I have found that instead of TopTransactionContext one
> can use PortalContext and get a more suitable life cycle for the memory.
>  I have played with some variants to make this configurable (e.g.,
> argument to SPI_connect()), but that didn't seem very useful.  There are
> some comments indicating that there might not always be a PortalContext,
> but the existing tests don't seem to mind.  (There was a thread recently
> about making a fake PortalContext for autovacuum, so maybe the current
> idea is that we make sure there always is a PortalContext.)  Maybe we
> need another context like StatementContext or ProcedureContext.

This could use more specific discussion, as it is a critical point.

One general theme in this patch is to use PortalContext instead of
TopTransactionContext (or similar).  In SPI_connect(), we have

/*
 * Create memory contexts for this procedure
 *
 * XXX it would be better to use PortalContext as the parent
context, but
 * we may not be inside a portal (consider deferred-trigger execution).
 * Perhaps CurTransactionContext would do?  For now it doesn't matter
 * because we clean up explicitly in AtEOSubXact_SPI().
 */
_SPI_current->procCxt = AllocSetContextCreate(TopTransactionContext,
  "SPI Proc",

and my patch changes that to PortalContext in defiance of that comment.

So either the comment is incorrect or we have insufficient test coverage
or something in between.

ISTM that in the normal case, at the time deferred triggers are
executed, we are in the portal that executes the COMMIT command, so that
should work.  There are some cases that call CommitTransactionCommand()
internally, but they don't run in cases when triggers are pending (e.g.,
VACUUM).  Although logical apply workers might be a problem, but they
clearly postdate that comment.

In any case, the precedent in autovacuum appears to be to make a fake
PortalContext if needed, so we could do that.  Can we think of other
cases where that might be necessary, so I can construct some test cases?

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Transaction control in procedures

[Simon Riggs]

On 18 November 2017 at 02:16, Peter Eisentraut
 wrote:
> On 11/16/17 18:35, Simon Riggs wrote:
>> For the first two answers above the answer was "currently executing
>> statement", yet the third answer seems to be the procedure. So that is
>> a slight discrepancy.
>
> That's the way function execution, or really any nested execution,
> currently works.

I'm impressed that these features are so clean and simple. I wasn't
expecting that. I have very few review comments.

I vote in favour of applying these patches at the end of this CF, end of 11/17.
* Procedures
* Transaction control in PL/pgSQL (only)

That will give us 3 months to discuss problems and find solutions,
then later we can commit PL/Python, PL/perl and PL/tcl once we know
where the dragons are hiding.

If we delay, we will end up with some weird gotcha that needs changing
in the next release.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Transaction control in procedures

[Peter Eisentraut]

On 11/16/17 18:35, Simon Riggs wrote:
> For the first two answers above the answer was "currently executing
> statement", yet the third answer seems to be the procedure. So that is
> a slight discrepancy.

That's the way function execution, or really any nested execution,
currently works.

> ISTM we would like
> 
> 1) a way to cancel execution of a procedure
> 2) a way to set a timeout to cancel execution of a procedure
> 
> as well as
> 
> 1) a way to cancel execution of a statement that is running within a procedure
> 2) a way to set a timeout to cancel execution of a statement in a procedure
> 
> Visibility of what a routine is currently executing is the role of a
> debugger utility/API.

That would probably be nice, but it would be an entirely separate
undertaking.  In particular, getting insight into some kind of execution
stack would necessarily be language specific.  We do have some of that
for PL/pgSQL of course.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Transaction control in procedures

[Merlin Moncure]

On Thu, Nov 16, 2017 at 5:35 PM, Simon Riggs  wrote:
> On 14 November 2017 at 13:09, Peter Eisentraut
>  wrote:
>
>>> *) Will pg_cancel_backend() cancel the currently executing statement
>>> or the procedure? (I guess probably the procedure but I'm curious)
>>
>> Same as the way it currently works.  It will raise an exception, which
>> will travel up the stack and eventually issue an error or be caught.  If
>> someone knows more specific concerns here I could look into it, but I
>> don't see any problem.
>>
>>> *) Will long running procedures be subject to statement timeout (and
>>> does it apply to the entire procedure)?
>>
>> See previous item.
>>
>>> Will they be able to control
>>> statement_timeout from within the procedure itself?
>>
>> The statement timeout alarm is set by the top-level execution loop, so
>> you can't change a statement timeout that is already in progress.  But
>> you could change the GUC and commit it for the next top-level statement.
>>
>>> *) Will pg_stat_activity show the invoking CALL or the currently
>>> executing statement?  I see a strong argument for showing both of
>>> these things. although I understand that's out of scope here.
>>
>> Not different from a function execution, i.e., top-level statement.
>
> Which is the "top-level statement"? The CALL or the currently
> executing statement within the proc? I think you mean former.
>
> For the first two answers above the answer was "currently executing
> statement", yet the third answer seems to be the procedure. So that is
> a slight discrepancy.
>
> ISTM we would like
>
> 1) a way to cancel execution of a procedure
> 2) a way to set a timeout to cancel execution of a procedure
>
> as well as
>
> 1) a way to cancel execution of a statement that is running within a procedure
> 2) a way to set a timeout to cancel execution of a statement in a procedure
>
> Visibility of what a routine is currently executing is the role of a
> debugger utility/API.

How could you cancel a statement but not the procedure itself?
Cancelling (either by timeout or administrative) type errors
untrappable by design for very good reasons and untrapped errors ought
to return the database all the way to 'ready for query'.

merlin

Re: [HACKERS] Transaction control in procedures

[Simon Riggs]

On 14 November 2017 at 13:09, Peter Eisentraut
 wrote:

>> *) Will pg_cancel_backend() cancel the currently executing statement
>> or the procedure? (I guess probably the procedure but I'm curious)
>
> Same as the way it currently works.  It will raise an exception, which
> will travel up the stack and eventually issue an error or be caught.  If
> someone knows more specific concerns here I could look into it, but I
> don't see any problem.
>
>> *) Will long running procedures be subject to statement timeout (and
>> does it apply to the entire procedure)?
>
> See previous item.
>
>> Will they be able to control
>> statement_timeout from within the procedure itself?
>
> The statement timeout alarm is set by the top-level execution loop, so
> you can't change a statement timeout that is already in progress.  But
> you could change the GUC and commit it for the next top-level statement.
>
>> *) Will pg_stat_activity show the invoking CALL or the currently
>> executing statement?  I see a strong argument for showing both of
>> these things. although I understand that's out of scope here.
>
> Not different from a function execution, i.e., top-level statement.

Which is the "top-level statement"? The CALL or the currently
executing statement within the proc? I think you mean former.

For the first two answers above the answer was "currently executing
statement", yet the third answer seems to be the procedure. So that is
a slight discrepancy.

ISTM we would like

1) a way to cancel execution of a procedure
2) a way to set a timeout to cancel execution of a procedure

as well as

1) a way to cancel execution of a statement that is running within a procedure
2) a way to set a timeout to cancel execution of a statement in a procedure

Visibility of what a routine is currently executing is the role of a
debugger utility/API.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Transaction control in procedures

[Simon Riggs]

On 15 November 2017 at 16:36, Peter Eisentraut
 wrote:
> On 11/8/17 18:48, Simon Riggs wrote:
>> What would happen if some of the INSERTs failed? Where would control
>> go to? (Maybe this is just "no change" in this particular proc)
>
> An exception is raised and unless the exception is caught (depending on
> the PL), control leaves the procedure.  What is already committed stays.
>
>> What happens if the procedure is updated during execution? Presumably
>> it keeps executing the original version as seen in the initial
>> snapshot?
>
> correct
>
>> Does the xmin of this session advance after each transaction, or do we
>> hold the snapshot used for the procedure body open, causing us to hold
>> back xmin and prevent vacuuming from being effective?
>>
>> What would happen if a procedure recursively called itself? And yet it
>> was updated half-way through? Would that throw an error (I think it
>> should).
>
> I don't think anything special happens here.  The snapshot that is used
> to read the procedure source code and other meta information is released
> at a transaction boundary.

I think we need to document that, or at least note in README

It's quite important for VACUUM.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Transaction control in procedures

[Merlin Moncure]

On Wed, Nov 15, 2017 at 3:42 PM, Peter Eisentraut
 wrote:
> On 11/15/17 09:54, Merlin Moncure wrote:
>> ... I noticed that:
>> *) now() did not advance with commit and,
>> *) xact_start via pg_stat_activity did not advance
>>
>> Shouldn't both of those advance with the in-loop COMMIT?
>
> I think you are correct.  I'll include that in the next patch version.
> It shouldn't be difficult.

Thanks.  A couple of more things.

*) This error message is incorrect now:
postgres=# CREATE OR REPLACE PROCEDURE foo() AS
$$
BEGIN
  LOOP
SAVEPOINT x;
  END LOOP;
END;
$$ LANGUAGE PLPGSQL;
CREATE PROCEDURE
Time: 0.912 ms
postgres=# call foo();
ERROR:  cannot begin/end transactions in PL/pgSQL
HINT:  Use a BEGIN block with an EXCEPTION clause instead.
CONTEXT:  PL/pgSQL function foo() line 5 at SQL statement

I guess there are a few places that assume pl/pgsql is always run from
a in-transaction function.

*) Exception handlers seem to override COMMITs.  The the following
procedure will not insert any rows.  I wonder if this is the correct
behavior.  I think there's a pretty good case to be made to raise an
error if a COMMIT is issued if you're in an exception block.

CREATE OR REPLACE PROCEDURE foo() AS
$$
BEGIN
  LOOP
INSERT INTO foo DEFAULT VALUES;
COMMIT;
RAISE EXCEPTION 'test';
  END LOOP;
EXCEPTION
  WHEN OTHERS THEN RAISE NOTICE '%', SQLERRM;
END;
$$ LANGUAGE PLPGSQL;

*) The documentation could use some work.  Would you like some help?

merlin

Re: [HACKERS] Transaction control in procedures

[Peter Eisentraut]

On 11/15/17 09:54, Merlin Moncure wrote:
> ... I noticed that:
> *) now() did not advance with commit and,
> *) xact_start via pg_stat_activity did not advance
> 
> Shouldn't both of those advance with the in-loop COMMIT?

I think you are correct.  I'll include that in the next patch version.
It shouldn't be difficult.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Transaction control in procedures

[Peter Eisentraut]

On 11/8/17 18:48, Simon Riggs wrote:
> What would happen if some of the INSERTs failed? Where would control
> go to? (Maybe this is just "no change" in this particular proc)

An exception is raised and unless the exception is caught (depending on
the PL), control leaves the procedure.  What is already committed stays.

> What happens if the procedure is updated during execution? Presumably
> it keeps executing the original version as seen in the initial
> snapshot?

correct

> Does the xmin of this session advance after each transaction, or do we
> hold the snapshot used for the procedure body open, causing us to hold
> back xmin and prevent vacuuming from being effective?
> 
> What would happen if a procedure recursively called itself? And yet it
> was updated half-way through? Would that throw an error (I think it
> should).

I don't think anything special happens here.  The snapshot that is used
to read the procedure source code and other meta information is released
at a transaction boundary.

>> 3) The PL implementations themselves allocate memory in
>> transaction-bound contexts for convenience as well.  This is usually
>> easy to fix by switching to PortalContext as well.  As you see, the
>> PL/Python code part of the patch is actually very small.  Changes in
>> other PLs would be similar.
> 
> Is there some kind of interlock to prevent dropping the portal half way thru?

I should probably look this up, but I don't think this is fundamentally
different from how VACUUM and CREATE INDEX CONCURRENTLY run inside a
portal and issue multiple transactions in sequence.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Transaction control in procedures

[Merlin Moncure]

On Wed, Nov 15, 2017 at 7:38 AM, Merlin Moncure  wrote:
> On Tue, Nov 14, 2017 at 5:27 PM, Peter Eisentraut
>>> Can we zero in on this?  The question implied, 'can you do this
>>> without being in a transaction'?  PERFORM do_stuff() is a implicit
>>> transaction, so it ought to end when the function returns right?
>>> Meaning, assuming I was not already in a transaction when hitting this
>>> block, I would not be subject to an endless transaction duration?
>>
>> In the server, you are always in a transaction, so that's not how this
>> works.  I think this also ties into my first response above.
>
> I'll try this out myself, but as long as we can have a *bounded*
> transaction lifetime (basically the time to do stuff + 1 second) via
> something like:
> LOOP
>   
>   COMMIT;
>   PERFORM pg_sleep(1);
> END LOOP;
>
> ... I'm good. I'll try your patch out ASAP.  Thanks for answering all
> my questions.


Trying this out (v2 both patches,  compiled clean, thank you!),
postgres=# CREATE OR REPLACE PROCEDURE foo() AS
$$
BEGIN
  LOOP
PERFORM 1;
COMMIT;
RAISE NOTICE '%', now();
PERFORM pg_sleep(1);
  END LOOP;
END;
$$ LANGUAGE PLPGSQL;
CREATE PROCEDURE
Time: 0.996 ms
postgres=# call foo();
NOTICE:  2017-11-15 08:52:08.936025-06
NOTICE:  2017-11-15 08:52:08.936025-06

... I noticed that:
*) now() did not advance with commit and,
*) xact_start via pg_stat_activity did not advance

Shouldn't both of those advance with the in-loop COMMIT?

merlin

Re: [HACKERS] Transaction control in procedures

[Pavel Stehule]

2017-11-15 14:38 GMT+01:00 Merlin Moncure :

> On Tue, Nov 14, 2017 at 5:27 PM, Peter Eisentraut
>  wrote:
> > On 11/14/17 16:33, Merlin Moncure wrote:
> >>> One detail in your example is that when you enter the procedure, you
> are
> >>> already in a transaction, so you would have to run either COMMIT or
> >>> ROLLBACK before the START TRANSACTION.
> >>
> >> Ok, that's good, but it seems a little wonky to me to have to issue
> >> COMMIT first.  Shouldn't that be the default?  Meaning you would not
> >> be *in* a transaction unless you specified to be in one.
> >
> > But that's not how this feature is defined in the SQL standard and AFAIK
> > other implementations.  When you enter the procedure call, you are in a
> > transaction.  For one thing, a procedure does not *have* to do
> > transaction control.  So if it's a plain old procedure like a function
> > that just runs a few statements, there needs to be a transaction.
>
> Hm, OK.   Well, SQL Server (which is pretty far from the SQL standard)
> works that way.  See here:
> http://www.4guysfromrolla.com/webtech/080305-1.shtml.  DB2, which is
> very close to the SQL standard, only supports COMMIT/ROLLBACK (not
> begin/start etc)
> https://www.ibm.com/support/knowledgecenter/en/SSULQD_7.2.
> 0/com.ibm.nz.sproc.doc/c_sproc_transaction_commits_and_rollbacks.html.
> Either approach is ok I guess, and always being in a transaction
> probably has some advantages. performance being an obvious one.  With
> DB2, the COMMIT statement acts as kind of a flush, or a paired
> 'commit;begin;'.
>

same in Oracle PL/SQL


> >> Can we zero in on this?  The question implied, 'can you do this
> >> without being in a transaction'?  PERFORM do_stuff() is a implicit
> >> transaction, so it ought to end when the function returns right?
> >> Meaning, assuming I was not already in a transaction when hitting this
> >> block, I would not be subject to an endless transaction duration?
> >
> > In the server, you are always in a transaction, so that's not how this
> > works.  I think this also ties into my first response above.
>
> I'll try this out myself, but as long as we can have a *bounded*
> transaction lifetime (basically the time to do stuff + 1 second) via
> something like:
> LOOP
>   
>   COMMIT;
>   PERFORM pg_sleep(1);
> END LOOP;
>
> ... I'm good. I'll try your patch out ASAP.  Thanks for answering all
> my questions.
>
> merlin
>
>

Re: [HACKERS] Transaction control in procedures

[Peter Eisentraut]

On 11/14/17 09:27, Merlin Moncure wrote:
> *) Will it be possible to do operations like this in pl/pgsql?
> 
> BEGIN
>   SELECT INTO r * FROM foo;
> 
>   START TRANSACTION;  -- perhaps we ought to have a special function
> for this instead (BEGIN is reserved, etc).
>   SET transaction_isololation TO serializable;
> ...

Eventually, I don't see why not.  Currently, it's not complete.

One detail in your example is that when you enter the procedure, you are
already in a transaction, so you would have to run either COMMIT or
ROLLBACK before the START TRANSACTION.

Also, you can't run SET TRANSACTION ISOLATION through SPI, so one would
have to implement a separate code path for that, but that would just be
a bit of leg work.

>  *) Will there be any negative consequences to a procedure running
> with an unbounded run time?  For example, something like:
> 
> LOOP
>   SELECT check_for_stuff_to_do();
> 
>   IF stuff_to_do
>   THEN
> do_stuff();
>   ELSE
> PERFORM pg_sleep(1);
>   END IF;
> END LOOP;

That procedure doesn't do anything with transactions, so it's just like
a long-running function.  Otherwise, it'd just be like long-running
client code managing transactions.

> *) Will pg_cancel_backend() cancel the currently executing statement
> or the procedure? (I guess probably the procedure but I'm curious)

Same as the way it currently works.  It will raise an exception, which
will travel up the stack and eventually issue an error or be caught.  If
someone knows more specific concerns here I could look into it, but I
don't see any problem.

> *) Will long running procedures be subject to statement timeout (and
> does it apply to the entire procedure)?

See previous item.

> Will they be able to control
> statement_timeout from within the procedure itself?

The statement timeout alarm is set by the top-level execution loop, so
you can't change a statement timeout that is already in progress.  But
you could change the GUC and commit it for the next top-level statement.

> *) Will pg_stat_activity show the invoking CALL or the currently
> executing statement?  I see a strong argument for showing both of
> these things. although I understand that's out of scope here.

Not different from a function execution, i.e., top-level statement.

> If these questions (especially the first two) come down the correct
> way, then it will mean that I can stop coding in other languages
> (primarily bash) for a fairly large number of cases that I really
> think belong in the database itself.  This would really simplify
> coding, some things in bash are really awkward to get right such as a
> mutex to guarantee single script invocation.  My only real dependency
> on the operation system environment at that point would be cron to
> step in to the backround daemon process (which would immediately set
> an advisory lock).

Well, some kind of scheduler around procedures would be pretty cool at
some point.

> I'm somewhat surprised that SPI is the point of attack for this
> functionality, but if it works that's really the best case scenario
> (the only downside I can see is that the various out of core pl/s have
> to implement the interface individually).

So I tried different things here, and I'll list them here to explain how
I got there.

Option zero is to not use SPI at all and implement a whole new internal
command execution system.  But that would obviously be a large amount of
work, and it would look 85% like SPI, and as it turns out it's not
necessary.

The first thing I tried out what to run transaction control statements
through SPI.  That turned out to be very complicated and confusing and
fragile, mainly because of the dichotomy between the internal
subtransaction management that the PLs do and the explicit transaction
control statements on the other hand.  It was just a giant unworkable mess.

The next thing I tried was to shut down (SPI_finish) SPI before a
transaction boundary command and restart it (SPI_connect) it afterwards.
 That would work in principle, but it would require a fair amount of
work in each PL, because they implicitly rely on SPI (or perhaps are
tangled up with SPI) for memory management.

The setup I finally arrived at was to implement the transaction boundary
commands as SPI API calls and let them internally make sure that only
the appropriate stuff is cleared away at transaction boundaries.  This
turned out to be the easiest and cleanest.  I have since the last patch
implemented the transaction control capabilities in PL/pgSQL, PL/Perl,
and PL/Tcl, and it was entirely trivial once the details were worked out
as I had shown in PL/Python.  I will post an updated patch with this soon.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Transaction control in procedures

[Merlin Moncure]

On Wed, Nov 8, 2017 at 5:48 PM, Simon Riggs  wrote:
> On 31 October 2017 at 15:38, Peter Eisentraut
>  wrote:
>> Here is a patch that implements transaction control in PL/Python
>> procedures.  (This patch goes on top of "SQL procedures" patch v1.)
>
> The patch is incredibly short for such a feature, which is probably a
> good indication that it is feasible.
>
> Amazing!

I have to agree with that.  I'm really excited about this...

Some questions:
*) Will it be possible to do operations like this in pl/pgsql?

BEGIN
  SELECT INTO r * FROM foo;

  START TRANSACTION;  -- perhaps we ought to have a special function
for this instead (BEGIN is reserved, etc).
  SET transaction_isololation TO serializable;
...

 *) Will there be any negative consequences to a procedure running
with an unbounded run time?  For example, something like:

LOOP
  SELECT check_for_stuff_to_do();

  IF stuff_to_do
  THEN
do_stuff();
  ELSE
PERFORM pg_sleep(1);
  END IF;
END LOOP;

*) Will pg_cancel_backend() cancel the currently executing statement
or the procedure? (I guess probably the procedure but I'm curious)

*) Will long running procedures be subject to statement timeout (and
does it apply to the entire procedure)?  Will they be able to control
statement_timeout from within the procedure itself?

*) Will pg_stat_activity show the invoking CALL or the currently
executing statement?  I see a strong argument for showing both of
these things. although I understand that's out of scope here.

If these questions (especially the first two) come down the correct
way, then it will mean that I can stop coding in other languages
(primarily bash) for a fairly large number of cases that I really
think belong in the database itself.  This would really simplify
coding, some things in bash are really awkward to get right such as a
mutex to guarantee single script invocation.  My only real dependency
on the operation system environment at that point would be cron to
step in to the backround daemon process (which would immediately set
an advisory lock).

I'm somewhat surprised that SPI is the point of attack for this
functionality, but if it works that's really the best case scenario
(the only downside I can see is that the various out of core pl/s have
to implement the interface individually).

merlin