On 03/28/2018 02:54 PM, Peter Eisentraut wrote:
> On 3/27/18 20:43, Tomas Vondra wrote:
>>>> 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.
>
> I put the formula into a separate variable isAtomicContext instead of
> repeating it twice. I think that makes it clearer. I'm not sure
> splitting it up like above makes it better, because the
> IsTransactionBlock() is part of the "is atomic". Maybe adding a comment
> would make it clearer.
>
I see. I thought "fixed" means you adopted the #define, but that's not
the case. I don't think an extra comment is needed - the variable name
is descriptive enough IMHO.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services