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-28 00:43:29 |
Message-ID: | ee156492-7b70-d41e-4575-1f7bf52e9b93@2ndquadrant.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Andres Freund | 2018-03-28 00:58:46 | Re: [HACKERS] logical decoding of two-phase transactions |
Previous Message | Tomas Vondra | 2018-03-28 00:34:17 | Re: [HACKERS] PATCH: multivariate histograms and MCV lists |