Re: Use merge-based matching for MCVs in eqjoinsel

Hi David,

Do you think anything else needs changes in the patch?

From an architectural perspective, I think the patch is already in good shape. However, I have some suggestions regarding code style:

1. I would move McvHashEntry, McvHashContext, he new hash table definition, hash_mcv and are_mcvs_equal to the top.
2. I’m not sure get_hash_func_oid() is needed at all – it seems we could do without it.
3. It would be better to name the parameters matchProductFrequencies -> matchprodfreq, nMatches -> nmatches, hasMatch1 -> hasmatch1, hasMatch2 -> hasmatch2 in eqjoinsel_inner_with_hashtable().
4. As I wrote earlier, since we now have a dedicated function eqjoinsel_inner_with_hashtable(), perhaps it could be used in both eqjoinsel_inner() and eqjoinsel_semi(). And since the hash-based search was factored out, maybe it would make sense to also factor out the O(N^2) nested loop implementation?
5. I think it would be helpful to add a comment explaining that using a hash table is not efficient when the MCV array length equals 1:

if (Min(statsSlot1->nvalues, statsSlot2->nvalues) == 1)
    return false;

Did you have a
chance to check tables with just few MCVs or are there any queries in
the JOB which regress with very small default_statistics_target?

Sure. I need some time to check this.

--
Best regards,
Ilia Evdokimov,
Tantor Labs LLC,
https://tantorlabs.com