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.
On its own, folding the sumcommon loop into the match-scan loop (my
earlier suggestion) isn't much of a speedup - max length of MCV list 10k
is small enough that the extra pass barely matters in the ordinary
no-match case. But now that scan_all_mcv_values can force a genuine
full-array scan, skipping the old separate sumcommon loop matters more
there: without the merge, a no-match under scan_all_mcv_values would
cost two full passes over the array instead of one. So I think it's
worth keeping in the patch for that reason, not for its standalone benefit.
A few remarks on v4 before:
1. pg_newlocale_from_collation(collation) runs whenever collation !=
sslot.stacoll, not only when one of them is actually nondeterministic -
and it builds a full pg_locale_t just to read one boolean.
get_collation_isdeterministic() in lsyscache.h reads
pg_collation.collisdeterministic straight off the syscache and is the
right-sized call for the question actually being asked here. I'd write
the block like this instead:
```
if (collation != sslot.stacoll && OidIsValid(collation) &&
!get_collation_isdeterministic(collation))
{
scan_all_mcv_values = !OidIsValid(sslot.stacoll) ||
get_collation_isdeterministic(sslot.stacoll);
}
```
That drops both pg_locale_t locals and keeps the OidIsValid guards that
fixed the collid == 0 crash.
2. Style nit, not a blocker: if (match == false) -> if (!match).
--
Best regards,
Ilia Evdokimov,
Tantor Labs LLC,
https://tantorlabs.com/
