Re: Making clausesel.c Smarter

On Mon, Apr 3, 2017 at 5:59 AM, David Rowley
<david(dot)rowley(at)2ndquadrant(dot)com> wrote:
>> +static void addCachedSelectivityRangeVar(CachedSelectivityClause
>> **cslist, Node *var,
>> bool varonleft, bool isLTsel, Selectivity s2);
>>
>> You're changing clause -> var throughout the code when dealing with
>> nodes, but looking at their use, they hold neither. All those
>> addCachedSelectivity functions are usually passed expression nodes. If
>> you're renaming, perhaps you should rename to "expr".
>
> hmm, this is true. I kind of followed the lead of the name of the
> variable in the old RangeQueryClause struct. I have changed the names
> of these to be expr in the attached, but I didn't change the name of
> the Var in the new CachedSelectivityClause struct. It seems like a
> change not related to this patch.
>
> Do you think I should change that too?

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;
+

>> I would suggest avoiding making the assumption that it doesn't unless
>> the operator is strict.
>>
>> One could argue that such an operator should provide its own
>> selectivity estimator, but the strictness check shouldn't be too
>> difficult to add, and I think it's a better choice.
>>
>> So you'd have to check that:
>>
>> default:
>> if (op_strict(expr->opno) && func_strict(expr->opfuncid))
>> addCachedSelectivityOtherClause(&cslist, var, s2);
>> else
>> s1 = s1 * s2;
>> break;
>>
>
> I've changed it to something along those lines. I don't think the
> func_strict here is required, though, so I've gone with just
> op_strict().

Indeed, I realized right after typing it, I thought I had corrected it
before I hit send. Seems I hadn't. Good thing you noticed.

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.