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
