Re: PL/pgSQL nested CALL with transactions

[Peter Eisentraut]

On 3/28/18 09:00, Tomas Vondra wrote:
> I see. I thought "fixed" means you adopted the #define, but that's not
> the case. I don't think an extra comment is needed - the variable name
> is descriptive enough IMHO.

Committed as presented then.  Thanks.

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

Re: PL/pgSQL nested CALL with transactions

[Tomas Vondra]

On 03/28/2018 02:54 PM, Peter Eisentraut wrote:
> On 3/27/18 20:43, Tomas Vondra wrote:
 3) utility.c

 I find this condition rather confusing:

 (!(context == PROCESS_UTILITY_TOPLEVEL ||
context == PROCESS_UTILITY_QUERY_NONATOMIC) ||
IsTransactionBlock())

 I mean, negated || with another || - it took me a while to parse what
 that means. I suggest doing this instead:

 #define ProcessUtilityIsAtomic(context)\
(!(context == PROCESS_UTILITY_TOPLEVEL ||
   context == PROCESS_UTILITY_QUERY_NONATOMIC))

 (ProcessUtilityIsAtomic(context) || IsTransactionBlock())
>>> fixed
>>>
>> Ummm, I still see the original code here.
> 
> I put the formula into a separate variable isAtomicContext instead of
> repeating it twice.  I think that makes it clearer.  I'm not sure
> splitting it up like above makes it better, because the
> IsTransactionBlock() is part of the "is atomic".  Maybe adding a comment
> would make it clearer.
> 

I see. I thought "fixed" means you adopted the #define, but that's not
the case. I don't think an extra comment is needed - the variable name
is descriptive enough IMHO.

regards

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

Re: PL/pgSQL nested CALL with transactions

[Peter Eisentraut]

On 3/27/18 20:43, Tomas Vondra wrote:
>>> 3) utility.c
>>>
>>> I find this condition rather confusing:
>>>
>>> (!(context == PROCESS_UTILITY_TOPLEVEL ||
>>>context == PROCESS_UTILITY_QUERY_NONATOMIC) ||
>>>IsTransactionBlock())
>>>
>>> I mean, negated || with another || - it took me a while to parse what
>>> that means. I suggest doing this instead:
>>>
>>> #define ProcessUtilityIsAtomic(context)\
>>>(!(context == PROCESS_UTILITY_TOPLEVEL ||
>>>   context == PROCESS_UTILITY_QUERY_NONATOMIC))
>>>
>>> (ProcessUtilityIsAtomic(context) || IsTransactionBlock())
>> fixed
>>
> Ummm, I still see the original code here.

I put the formula into a separate variable isAtomicContext instead of
repeating it twice.  I think that makes it clearer.  I'm not sure
splitting it up like above makes it better, because the
IsTransactionBlock() is part of the "is atomic".  Maybe adding a comment
would make it clearer.

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

Re: PL/pgSQL nested CALL with transactions

[Tomas Vondra]

On 03/24/2018 03:14 PM, Peter Eisentraut wrote:
> On 3/22/18 11:50, Tomas Vondra wrote:
> 
>> 2) spi.c
>>
>> I generally find it confusing when there are negative flags, which are
>> then negated whenever used. That is pretty the "no_snapshots" case,
>> because it means "don't manage snapshots" and all references to the flag
>> look like this:
>>
>> if (snapshot != InvalidSnapshot && !plan->no_snapshots)
>>
>> Why not to have "positive" flag instead, e.g. "manage_snapshots"?
> 
> Yeah, I'm not too fond of this either.  But there is a bunch of code in
> spi.c that assumes that it can initialize an _SPI_plan to all zeros to
> get it into a default state.  (See all the memset() calls.)  If we
> flipped this struct field around, we would have to change all those
> places, and all future such places, leading to potential headaches.
> 

Oh, I see :-( Yeah, then it does not make sense to change this.

>> FWIW the comment in_SPI_execute_plan talking about "four distinct
>> snapshot management behaviors" should mention that with
>> no_snapshots=true none of those really matters.
> 
> done
> 
>> I also wonder why _SPI_make_plan_non_temp/_SPI_save_plan changes palloc
>> to palloc0. It is just to initialize the no_snapshots flag explicitly?
>> It seems a bit wasteful to do a memset and then go and initialize all
>> the fields anyway, although I don't know how sensitive this code is.
> 
> As mentioned above, this actually makes it a bit more consistent with
> all the memset() calls.  I have updated the new patch to remove the
> now-redundant initializations.
> 

Yeah, now it makes sense.

>> 3) utility.c
>>
>> I find this condition rather confusing:
>>
>> (!(context == PROCESS_UTILITY_TOPLEVEL ||
>>context == PROCESS_UTILITY_QUERY_NONATOMIC) ||
>>IsTransactionBlock())
>>
>> I mean, negated || with another || - it took me a while to parse what
>> that means. I suggest doing this instead:
>>
>> #define ProcessUtilityIsAtomic(context)\
>>(!(context == PROCESS_UTILITY_TOPLEVEL ||
>>   context == PROCESS_UTILITY_QUERY_NONATOMIC))
>>
>> (ProcessUtilityIsAtomic(context) || IsTransactionBlock())
> 
> fixed
> 

Ummm, I still see the original code here.


The rest of the changes seems OK to me.

regards

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

Re: PL/pgSQL nested CALL with transactions

[Peter Eisentraut]

On 3/22/18 11:50, Tomas Vondra wrote:
> 1) plpgsql.sgml
> 
> The new paragraph talks about "intervening command" - I've been unsure
> what that refers too, until I read the comment for ExecutCallStmt(),
> which explains this as CALL -> SELECT -> CALL. Perhaps we should add
> that to the sgml docs too.

done

> 2) spi.c
> 
> I generally find it confusing when there are negative flags, which are
> then negated whenever used. That is pretty the "no_snapshots" case,
> because it means "don't manage snapshots" and all references to the flag
> look like this:
> 
> if (snapshot != InvalidSnapshot && !plan->no_snapshots)
> 
> Why not to have "positive" flag instead, e.g. "manage_snapshots"?

Yeah, I'm not too fond of this either.  But there is a bunch of code in
spi.c that assumes that it can initialize an _SPI_plan to all zeros to
get it into a default state.  (See all the memset() calls.)  If we
flipped this struct field around, we would have to change all those
places, and all future such places, leading to potential headaches.

> FWIW the comment in_SPI_execute_plan talking about "four distinct
> snapshot management behaviors" should mention that with
> no_snapshots=true none of those really matters.

done

> I also wonder why _SPI_make_plan_non_temp/_SPI_save_plan changes palloc
> to palloc0. It is just to initialize the no_snapshots flag explicitly?
> It seems a bit wasteful to do a memset and then go and initialize all
> the fields anyway, although I don't know how sensitive this code is.

As mentioned above, this actually makes it a bit more consistent with
all the memset() calls.  I have updated the new patch to remove the
now-redundant initializations.

> 3) utility.c
> 
> I find this condition rather confusing:
> 
> (!(context == PROCESS_UTILITY_TOPLEVEL ||
>context == PROCESS_UTILITY_QUERY_NONATOMIC) ||
>IsTransactionBlock())
> 
> I mean, negated || with another || - it took me a while to parse what
> that means. I suggest doing this instead:
> 
> #define ProcessUtilityIsAtomic(context)\
>(!(context == PROCESS_UTILITY_TOPLEVEL ||
>   context == PROCESS_UTILITY_QUERY_NONATOMIC))
> 
> (ProcessUtilityIsAtomic(context) || IsTransactionBlock())

fixed

> 4) spi_priv.h
> 
> Shouldn't the comment for _SPI_plan also mention what no_snapshots does?

There is a comment for the field.  You mean the comment at the top?  I
think it's OK the way it is.

> 5) utility.h
> 
> So now that we have  PROCESS_UTILITY_QUERY and
> PROCESS_UTILITY_QUERY_NONATOMIC, does that mean PROCESS_UTILITY_QUERY is
> always atomic?

Yes, that's just the pre-existing behavior.

> 6) pl_exec.h
> 
> The exec_prepare_plan in exec_stmt_perform was expanded into a local
> copy of the code, but ISTM the new code just removes handling of some
> error types when (plan==NULL), and doesn't call SPI_keepplan or
> exec_simple_check_plan. Why not to keep using exec_prepare_plan and add
> a new parameter to skip those calls?

Good idea.  Done.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 48e703670579b5a5049a0638a156975178d71ed5 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pete...@gmx.net>
Date: Sat, 24 Mar 2018 10:05:06 -0400
Subject: [PATCH v3] PL/pgSQL: Nested CALL with transactions

So far, a nested CALL or DO in PL/pgSQL would not establish a context
where transaction control statements were allowed.  This fixes that by
handling CALL and DO specially in PL/pgSQL, passing the atomic/nonatomic
execution context through and doing the required management around
transaction boundaries.
---
 doc/src/sgml/plpgsql.sgml  | 20 +-
 src/backend/executor/spi.c | 34 +
 src/backend/tcop/utility.c |  2 +-
 src/include/executor/spi_priv.h|  1 +
 src/include/tcop/utility.h |  1 +
 .../plpgsql/src/expected/plpgsql_transaction.out   | 83 +-
 src/pl/plpgsql/src/pl_exec.c   | 69 +-
 src/pl/plpgsql/src/pl_funcs.c  |  4 +-
 src/pl/plpgsql/src/pl_gram.y   | 18 +
 src/pl/plpgsql/src/pl_handler.c|  4 +-
 src/pl/plpgsql/src/pl_scanner.c|  1 +
 src/pl/plpgsql/src/plpgsql.h   |  5 +-
 src/pl/plpgsql/src/sql/plpgsql_transaction.sql | 52 --
 13 files changed, 235 insertions(+), 59 deletions(-)

diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 7ed926fd51..b63b8496c7 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -3463,9 +3463,9 @@ Looping Thr

Re: PL/pgSQL nested CALL with transactions

[Tomas Vondra]

Hi,

The patch looks good to me - certainly in the sense that I haven't found
any bugs during the review. I do have a couple of questions and comments
about possible improvements, though. Some of this may be a case of
bike-shedding or simply because I've not looked as SPI/plpgsql before.


1) plpgsql.sgml

The new paragraph talks about "intervening command" - I've been unsure
what that refers too, until I read the comment for ExecutCallStmt(),
which explains this as CALL -> SELECT -> CALL. Perhaps we should add
that to the sgml docs too.


2) spi.c

I generally find it confusing when there are negative flags, which are
then negated whenever used. That is pretty the "no_snapshots" case,
because it means "don't manage snapshots" and all references to the flag
look like this:

if (snapshot != InvalidSnapshot && !plan->no_snapshots)

Why not to have "positive" flag instead, e.g. "manage_snapshots"?

FWIW the comment in_SPI_execute_plan talking about "four distinct
snapshot management behaviors" should mention that with
no_snapshots=true none of those really matters.

I also wonder why _SPI_make_plan_non_temp/_SPI_save_plan changes palloc
to palloc0. It is just to initialize the no_snapshots flag explicitly?
It seems a bit wasteful to do a memset and then go and initialize all
the fields anyway, although I don't know how sensitive this code is.


3) utility.c

I find this condition rather confusing:

(!(context == PROCESS_UTILITY_TOPLEVEL ||
   context == PROCESS_UTILITY_QUERY_NONATOMIC) ||
   IsTransactionBlock())

I mean, negated || with another || - it took me a while to parse what
that means. I suggest doing this instead:

#define ProcessUtilityIsAtomic(context)\
   (!(context == PROCESS_UTILITY_TOPLEVEL ||
  context == PROCESS_UTILITY_QUERY_NONATOMIC))

(ProcessUtilityIsAtomic(context) || IsTransactionBlock())


4) spi_priv.h

Shouldn't the comment for _SPI_plan also mention what no_snapshots does?


5) utility.h

So now that we have  PROCESS_UTILITY_QUERY and
PROCESS_UTILITY_QUERY_NONATOMIC, does that mean PROCESS_UTILITY_QUERY is
always atomic?

6) pl_exec.h

The exec_prepare_plan in exec_stmt_perform was expanded into a local
copy of the code, but ISTM the new code just removes handling of some
error types when (plan==NULL), and doesn't call SPI_keepplan or
exec_simple_check_plan. Why not to keep using exec_prepare_plan and add
a new parameter to skip those calls?


regards

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

Re: PL/pgSQL nested CALL with transactions

[Pavel Stehule]

2018-03-16 18:49 GMT+01:00 Tom Lane :

> Pavel Stehule  writes:
> > 2018-03-16 18:35 GMT+01:00 Peter Eisentraut <
> > peter.eisentr...@2ndquadrant.com>:
> >> Not very typical, but we apply the same execution context handling to
> >> CALL and DO at the top level, so it would be weird not to propagate
> that.
>
> > Although it is possible, I don't see any sense of introduction for DO
> into
> > plpgsql. Looks like duplicate to EXECUTE.
>
> Not sure what you think is being "introduced" here.  It already works just
> like any other random SQL command:
>
> regression=# do $$
> regression$# begin
> regression$#   raise notice 'outer';
> regression$#   do $i$ begin raise notice 'inner'; end $i$;
> regression$# end $$;
> NOTICE:  outer
> NOTICE:  inner
> DO
>
> While certainly that's a bit silly as-is, I think it could have practical
> use if the inner DO invokes a different PL.
>

ok, make sense

Pavel

>
> regards, tom lane
>

Re: PL/pgSQL nested CALL with transactions

[Tom Lane]

Pavel Stehule  writes:
> 2018-03-16 18:35 GMT+01:00 Peter Eisentraut <
> peter.eisentr...@2ndquadrant.com>:
>> Not very typical, but we apply the same execution context handling to
>> CALL and DO at the top level, so it would be weird not to propagate that.

> Although it is possible, I don't see any sense of introduction for DO into
> plpgsql. Looks like duplicate to EXECUTE.

Not sure what you think is being "introduced" here.  It already works just
like any other random SQL command:

regression=# do $$
regression$# begin
regression$#   raise notice 'outer';
regression$#   do $i$ begin raise notice 'inner'; end $i$;
regression$# end $$;
NOTICE:  outer
NOTICE:  inner
DO

While certainly that's a bit silly as-is, I think it could have practical
use if the inner DO invokes a different PL.

regards, tom lane

Re: PL/pgSQL nested CALL with transactions

[Pavel Stehule]

2018-03-16 18:35 GMT+01:00 Peter Eisentraut <
peter.eisentr...@2ndquadrant.com>:

> On 3/16/18 00:24, Pavel Stehule wrote:
> > What is benefit of DO command in PLpgSQL? Looks bizarre for me.
>
> Not very typical, but we apply the same execution context handling to
> CALL and DO at the top level, so it would be weird not to propagate that.
>

Although it is possible, I don't see any sense of introduction for DO into
plpgsql. Looks like duplicate to EXECUTE.



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

Re: PL/pgSQL nested CALL with transactions

[Peter Eisentraut]

On 3/16/18 00:24, Pavel Stehule wrote:
> What is benefit of DO command in PLpgSQL? Looks bizarre for me.

Not very typical, but we apply the same execution context handling to
CALL and DO at the top level, so it would be weird not to propagate that.

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

Re: PL/pgSQL nested CALL with transactions

[Pavel Stehule]

Hi

2018-03-16 2:57 GMT+01:00 Peter Eisentraut :

> On 2/28/18 14:51, Peter Eisentraut wrote:
> > So far, a nested CALL or DO in PL/pgSQL would not establish a context
> > where transaction control statements were allowed.  This patch fixes
> > that by handling CALL and DO specially in PL/pgSQL, passing the
> > atomic/nonatomic execution context through and doing the required
> > management around transaction boundaries.
>
> rebased patch
>

What is benefit of DO command in PLpgSQL? Looks bizarre for me.

Reards

Pavel


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

Re: PL/pgSQL nested CALL with transactions

[Peter Eisentraut]

On 2/28/18 14:51, Peter Eisentraut wrote:
> So far, a nested CALL or DO in PL/pgSQL would not establish a context
> where transaction control statements were allowed.  This patch fixes
> that by handling CALL and DO specially in PL/pgSQL, passing the
> atomic/nonatomic execution context through and doing the required
> management around transaction boundaries.

rebased patch

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 2254a08df89db4191ed63753f95bb8fbda5ef150 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pete...@gmx.net>
Date: Thu, 15 Mar 2018 21:54:28 -0400
Subject: [PATCH v2] PL/pgSQL: Nested CALL with transactions

So far, a nested CALL or DO in PL/pgSQL would not establish a context
where transaction control statements were allowed.  This fixes that by
handling CALL and DO specially in PL/pgSQL, passing the atomic/nonatomic
execution context through and doing the required management around
transaction boundaries.

XXX This requires a new flag in SPI to run SPI_execute* without snapshot
management.  This currently poked directly into the plan struct,
requiring access to spi_priv.h.  This could use some refinement ideas.
---
 doc/src/sgml/plpgsql.sgml  | 13 +++-
 src/backend/executor/spi.c | 12 ++--
 src/backend/tcop/utility.c |  4 +-
 src/include/executor/spi_priv.h|  1 +
 src/include/tcop/utility.h |  1 +
 .../plpgsql/src/expected/plpgsql_transaction.out   | 71 +-
 src/pl/plpgsql/src/pl_exec.c   | 56 +++--
 src/pl/plpgsql/src/pl_funcs.c  |  4 +-
 src/pl/plpgsql/src/pl_gram.y   | 18 ++
 src/pl/plpgsql/src/pl_handler.c|  4 +-
 src/pl/plpgsql/src/pl_scanner.c|  1 +
 src/pl/plpgsql/src/plpgsql.h   |  5 +-
 src/pl/plpgsql/src/sql/plpgsql_transaction.sql | 42 +++--
 13 files changed, 188 insertions(+), 44 deletions(-)

diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 6c25116538..ba3117c733 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -3463,9 +3463,9 @@ Looping Through a Cursor's Result
Transaction Management
 

-In procedures invoked by the CALL command from the top
-level as well as in anonymous code blocks (DO command)
-called from the top level, it is possible to end transactions using the
+In procedures invoked by the CALL command
+as well as in anonymous code blocks (DO command),
+it is possible to end transactions using the
 commands COMMIT and ROLLBACK.  A new
 transaction is started automatically after a transaction is ended using
 these commands, so there is no separate START
@@ -3495,6 +3495,13 @@ Transaction Management
 

 
+   
+Transaction control is only possible in CALL or
+DO invocations from the top level or nested
+CALL or DO invocations without any
+other intervening command.
+   
+

 A transaction cannot be ended inside a loop over a query result, nor
 inside a block with exception handlers.
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 9fc4431b80..174ec6cd5a 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -2042,7 +2042,7 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
 * In the first two cases, we can just push the snap onto the stack once
 * for the whole plan list.
 */
-   if (snapshot != InvalidSnapshot)
+   if (snapshot != InvalidSnapshot && !plan->no_snapshots)
{
if (read_only)
{
@@ -2121,7 +2121,7 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
 * In the default non-read-only case, get a new snapshot, 
replacing
 * any that we pushed in a previous cycle.
 */
-   if (snapshot == InvalidSnapshot && !read_only)
+   if (snapshot == InvalidSnapshot && !read_only && 
!plan->no_snapshots)
{
if (pushed_active_snap)
PopActiveSnapshot();
@@ -2172,7 +2172,7 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
 * If not read-only mode, advance the command counter 
before each
 * command and update the snapshot.
 */
-   if (!read_only)
+   if (!read_only && !plan->no_snapshots)
{
CommandCounterIncrement();
UpdateActiveSnapshotCommandId();
@@ -2206,7 +2206,7 @@ _SPI_execute_plan(SPIPlanPtr plan, Pa

PL/pgSQL nested CALL with transactions

[Peter Eisentraut]

So far, a nested CALL or DO in PL/pgSQL would not establish a context
where transaction control statements were allowed.  This patch fixes
that by handling CALL and DO specially in PL/pgSQL, passing the
atomic/nonatomic execution context through and doing the required
management around transaction boundaries.

This requires a new flag in SPI to run SPI_execute* without snapshot
management.  This currently poked directly into the plan struct,
requiring access to spi_priv.h.  This could use some refinement ideas.

Other PLs are currently not supported, but they could be extended in the
future using the principles being worked out here.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From fa13c429d1b401643af4fa87f553784edaa1515a Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pete...@gmx.net>
Date: Wed, 28 Feb 2018 14:50:11 -0500
Subject: [PATCH v1] PL/pgSQL: Nested CALL with transactions

So far, a nested CALL or DO in PL/pgSQL would not establish a context
where transaction control statements were allowed.  This fixes that by
handling CALL and DO specially in PL/pgSQL, passing the atomic/nonatomic
execution context through and doing the required management around
transaction boundaries.

XXX This requires a new flag in SPI to run SPI_execute* without snapshot
management.  This currently poked directly into the plan struct,
requiring access to spi_priv.h.  This could use some refinement ideas.
---
 doc/src/sgml/plpgsql.sgml  | 13 +++-
 src/backend/executor/spi.c | 12 +--
 src/backend/tcop/utility.c |  4 +-
 src/include/executor/spi_priv.h|  1 +
 src/include/tcop/utility.h |  1 +
 .../plpgsql/src/expected/plpgsql_transaction.out   | 73 +-
 src/pl/plpgsql/src/pl_exec.c   | 86 --
 src/pl/plpgsql/src/pl_funcs.c  | 25 +++
 src/pl/plpgsql/src/pl_gram.y   | 37 +-
 src/pl/plpgsql/src/pl_handler.c|  4 +-
 src/pl/plpgsql/src/pl_scanner.c|  2 +
 src/pl/plpgsql/src/plpgsql.h   | 16 +++-
 src/pl/plpgsql/src/sql/plpgsql_transaction.sql | 42 +--
 13 files changed, 273 insertions(+), 43 deletions(-)

diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index c1e3c6a19d..04aa1759cd 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -3447,9 +3447,9 @@ Looping Through a Cursor's Result
Transaction Management
 

-In procedures invoked by the CALL command from the top
-level as well as in anonymous code blocks (DO command)
-called from the top level, it is possible to end transactions using the
+In procedures invoked by the CALL command
+as well as in anonymous code blocks (DO command),
+it is possible to end transactions using the
 commands COMMIT and ROLLBACK.  A new
 transaction is started automatically after a transaction is ended using
 these commands, so there is no separate START
@@ -3479,6 +3479,13 @@ Transaction Management
 

 
+   
+Transaction control is only possible in CALL or
+DO invocations from the top level or nested
+CALL or DO invocations without any
+other intervening command.
+   
+

 A transaction cannot be ended inside a loop over a query result, nor
 inside a block with exception handlers.
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 9fc4431b80..174ec6cd5a 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -2042,7 +2042,7 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
 * In the first two cases, we can just push the snap onto the stack once
 * for the whole plan list.
 */
-   if (snapshot != InvalidSnapshot)
+   if (snapshot != InvalidSnapshot && !plan->no_snapshots)
{
if (read_only)
{
@@ -2121,7 +2121,7 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
 * In the default non-read-only case, get a new snapshot, 
replacing
 * any that we pushed in a previous cycle.
 */
-   if (snapshot == InvalidSnapshot && !read_only)
+   if (snapshot == InvalidSnapshot && !read_only && 
!plan->no_snapshots)
{
if (pushed_active_snap)
PopActiveSnapshot();
@@ -2172,7 +2172,7 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
 * If not read-only mode, advance the command counter 
before each
 * command and update the snapshot.
 */
-   if (!read_only)
+