Re: Fix var_eq_const: sum selectivity of all matching MCV entries instead of stopping at first match

Ilia Evdokimov <ilya(dot)evdokimov(at)tantorlabs(dot)com> writes:
> I reviewed v4-patch. If this is only for that specific case
> (deterministic column stats, nondeterministic comparison collation),
> then scanning the whole MCV array instead of stopping at the first match
> seems fine to me.

I think there's some muddled thinking in this patch. The two cases
that seem of interest to me are:

1. The query operator uses a deterministic collation. It will match
at most one MCV entry, regardless of what collation was used to make
the stats entry. If the collation used to make the entry was
nondeterministic, it may have merged value(s) that the query operator
would think were equal with values that it would think aren't equal,
but we have no way to sort that out, so we might as well disregard
the issue. Not using the MCV array would not improve our estimate,
nor would scanning to the end.

2. The query operator uses a nondeterministic collation. If that
collation is identical to that used to make the stats entry, then
there will still be at most one matching MCV entry. Otherwise the
query operator may match more than one MCV entry, regardless of which
other collation was used to make the stats entry. So we should scan
to the end. Again, the result's accuracy is fuzzier than one could
wish if the other collation was nondeterministic, and again there's
little we can do about that, so why worry?

In short then, I don't believe the criterion proposed in the patch:
I don't see why it makes any difference to this code whether the
stats' collation was deterministic.

> ... I'd write
> the block like this instead:
> ```
> if (collation != sslot.stacoll && OidIsValid(collation) &&
>    !get_collation_isdeterministic(collation))

I could go either way on that. There's already code in selfuncs.c
that depends on pg_newlocale_from_collation, and none that depends
on get_collation_isdeterministic, so arguably this way is more
consistent. Another point is that given all the caching that happens
underneath pg_newlocale_from_collation, it's very likely faster than
a catcache lookup. (I didn't test that claim though.)

In general I'm not too pleased with the quality of the comments
in this patch: it removes some good ones and doesn't replace them,
and the reasoning for whether we need to scan to the end isn't
well explained.

regards, tom lane