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
