From: | Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com> |
---|---|
To: | Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: PL/pgSQL nested CALL with transactions |
Date: | 2018-03-22 15:50:56 |
Message-ID: | f6b3ce3b-910d-af72-8cd0-dd77b1b62f0a@2ndquadrant.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Pavel Stehule | 2018-03-22 15:57:30 | Re: Re: csv format for psql |
Previous Message | Bossart, Nathan | 2018-03-22 15:45:23 | Re: Change RangeVarGetRelidExtended() to take flags argument? |