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
