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
