Original >From: Tom Lane <[email protected]> >Date: 2026-09-09 06:04 >To: Ilia Evdokimov <[email protected]> >Cc: ZizhuanLiu X-MAN <[email protected]>, pgsql-hackers ><[email protected]>, Damil Shahzad <[email protected]> >Subject: Re: Fix var_eq_const: sum selectivity of all matching MCV entries >instead of stopping at first match >Ilia Evdokimov <[email protected]> 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)
[email protected]
v5-0001-Fix-var_eq_const-sum-selectivity-of-all-matching-.patch
Description: Binary data
