Re: Use merge-based matching for MCVs in eqjoinsel

Ilia Evdokimov <ilya.evdokimov@tantorlabs.com> writes:
> On 14.11.2025 01:21, Tom Lane wrote:
>> So after a bit of hacking I ended up with the attached.  I split up
>> the refactorization into several steps to make it easier to review.

> I reviewed patches 0002-0004 with the refactoring, and I think the 
> overall approach is excellent. However, I noticed one issue: in 
> eqjoinsel_semi() the variable 'nmatches' is not initialized and can lead 
> to undefined behavior when clamped_nvalues2 == sslot2->nvalues. Before 
> the refactoring it was initialized by zero.

Argh!  Can't believe I missed that.

I experimented with combining all of eqjoinsel_find_matches' outputs
into one struct, but decided that that was uglier than just adding one
more pass-by-reference argument to eqjoinsel_inner/semi.

>> There are still a couple of XXX comments in the attached, denoting
>> loose ends to look at.

In the attached v6, I cleaned up one of the XXX items, deciding that
duplicate entries in the hash table should be coped with not asserted
against.  The reason is that we might be working with a comparison
operator that is not exactly the one used to build the MCV list:
the planner is usually pretty cavalier about applying stats that are
only fuzzy matches to what it needs.  So it seems possible that we
could find entries that are equal according to the operator we're
using, even though they're unequal according to what ANALYZE used to
compute the MCV list.  I didn't actively try to hit that Assert, but
I think a counterexample could be built by using a case-insensitive
collation in a join query.

>> In particular, I wondered whether the
>> hash threshold check
>> if (Min(sslot1.nvalues, sslot2.nvalues) >= EQJOINSEL_MCV_HASH_THRESHOLD)
>> should use Max() instead --- that is, it might be safer to hash
>> if either MCV list is long.  Or, holding one's head at a different
>> angle, perhaps the sum of the list lengths should be what's checked?

> Hmm… using the sum actually seems like a good idea for me.

Actually, after sleeping on it it seems like the obvious thing is
to test "sslot1.nvalues * sslot2.nvalues", since the work we are
thinking about saving scales as that product.  But I'm not sure
what threshold value to use if we do that.  Maybe around 10000?

			regards, tom lane