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
