On 4 April 2017 at 11:35, Claudio Freire <klaussfre...@gmail.com> wrote: > I'd prefer it if all occurrences of the concept were changed, to > maintain consistency. > That would include variables used to hold expressions that refer to > these as well, as in the case of: > > + Node *var; > + > + if (varonleft) > + var = leftexpr; > + else > + var = rightexpr; > +
Thanks for looking again. I didn't change that one as there's already a variable named "expr" in the scope. I thought changing that would mean code churn that I don't really want to add to the patch. Someone else might come along and ask me why I'm renaming this unrelated variable. I kinda of think that if it was var before when it meant expr, then it's not the business of this patch to clean that up. I didn't rename the struct member, for example, as the meaning is no different than before. > One last observation: > > + /* > + * An IS NOT NULL test is a no-op if there's any other strict quals, > + * so if that's the case, then we'll only apply this, otherwise we'll > + * ignore it. > + */ > + else if (cslist->selmask == CACHESEL_NOTNULLTEST) > + s1 *= cslist->notnullsel; > > In some paths, the range selectivity estimation code punts and returns > DEFAULT_RANGE_INEQ_SEL. In those cases, if a not null test was > present, it should still be applied. It could make a big difference if > the selectivity for the nulltest is high. I'd say that the location that applies the DEFAULT_RANGE_INEQ_SEL should apply the nullfrac. Seems wrong to rely on a IS NOT NULL test to exists to estimate that properly. I don't think that needs done as part of this patch. I'd rather limit the scope since "returned with feedback" is already knocking at the door of this patch. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
