PL/pgSQL nested CALL with transactions

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

Hi

2018-03-16 2:57 GMT+01:00 Peter Eisentraut <peter.eisentraut@2ndquadrant.com

:

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

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

2018-03-16 18:35 GMT+01:00 Peter Eisentraut <
peter.eisentraut@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.

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

2018-03-16 18:49 GMT+01:00 Tom Lane <tgl@sss.pgh.pa.us>:

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.

ok, make sense

Pavel

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