Re: Making clausesel.c Smarter

On 4 April 2017 at 11:35, Claudio Freire <klaussfreire(at)gmail(dot)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