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

From: Ilia Evdokimov <ilya(dot)evdokimov(at)tantorlabs(dot)com>
To: ZizhuanLiu X-MAN <44973863(at)qq(dot)com>, pgsql-hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Cc: Damil Shahzad <shahzaddamil(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Subject: Re: Fix var_eq_const: sum selectivity of all matching MCV entries instead of stopping at first match
Date: 2026-09-08 20:33:17
Message-ID: 63af64e5-f96b-4930-91dc-a6988f0c8398@tantorlabs.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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/

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Zsolt Parragi 2026-09-08 20:42:55 Re: Introducing find_all_inheritors_ordered()
Previous Message Andres Freund 2026-09-08 20:09:27 Re: Assert in test_bms_membership();