Thread: 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 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

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

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

Pavel Stehule <pavel.stehule@gmail.com> writes:
> 2018-03-16 18:35 GMT+01:00 Peter Eisentraut <
> peter.eisentraut@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

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

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

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

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

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

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