Original
>From: Tom Lane <tgl@sss.pgh.pa.us>
>Date: 2026-09-09 06:04
>To: Ilia Evdokimov <ilya.evdokimov@tantorlabs.com>
>Cc: ZizhuanLiu X-MAN <44973863@qq.com>, pgsql-hackers <pgsql-hackers@lists.postgresql.org>, Damil Shahzad
<shahzaddamil@gmail.com>
>Subject: Re: Fix var_eq_const: sum selectivity of all matching MCV entries instead of stopping at first match
>Ilia Evdokimov <ilya.evdokimov@tantorlabs.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
Hi, Ilia, tom
Thanks everyone for your time reviewing and for the valuable feedback.
I agree with the overall suggestions. Attached is the v5 patch.
Changes relative to v4:
- Fix the condition to trigger full MCV-array scan:
enable it only when the comparison collation is nondeterministic and differs from the statistics collation.
- Enhance comments for non-trivial logic.
- Keep pg_newlocale_from_collation() in order to leverage its caching.
- Rename variable to scan_entire_mcv, plus minor code-layout tweaks.
- No other functional changes. Reviews and feedback are welcome.
regards,
--
ZizhuanLiu (X-MAN)
44973863@qq.com
