Re: New design for FK-based join selectivity estimation

Hi,

On 06/16/2016 01:00 AM, Tom Lane wrote:
> Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
>> Attached is a reworked patch, mostly following the new design proposal
>> from this thread.
>
> I've whacked this around quite a bit and am now reasonably happy with it.
> The issue of planning speed hit should be pretty much gone, although I've
> not done testing to prove that.  I've rearranged the actual selectivity
> calculation some too, because tests I did here did not look very good for
> anything but the plain-inner-join case.  It may be that more work is
> needed there, but at least there's reasonable commentary about what we're
> doing and why.

Thanks for getting the patch into a much better shape. I've quickly 
reviewed the patch this morning before leaving to the airport - I do 
plan to do additional review/testing once in the air or perhaps over the 
weekend. So at the moment I only have a few minor comments:

1) Shouldn't we define a new macro in copyfuncs.c, to do the memcpy for 
us? Seems a bit strange we have macros for everything else.

2) I'm wondering whether removing the restrict infos when
    if (fkinfo->eclass[i] == rinfo->parent_ec)

is actually correct. Can't the EC include conditions that do not match 
the FK? I mean something like this:
  CREATE TABLE a (id1 INT PRIMARY KEY, id2 INT);  CREATE TABLE b (id1 INT REFERENCES a (id1), id2 INT);

and then something like
  SELECT * FROM a JOIN b ON (a.id1 = b.id1 AND a.id1 = b.id2)

I've been unable to trigger this issue with your patch, but I do 
remember I've ran into that with my version, which is why I explicitly 
checked the rinfo again before removing it. Maybe that was incorrect, or 
perhaps your patch does something smart. Or maybe it's just masked by 
the fact that we 'push' the EC conditions to the base relations (which 
however means we're stuck with default equality estimate).

3) I think this comment in get_foreign_key_join_selectivity is wrong and 
should instead say 'to FK':
  /* Otherwise, see if rinfo was previously matched to EC */

>
> I have not adopted the plan of ignoring single-column FKs.  While it would
> only take a couple more lines to do so, I think the argument that there
> will be a material planning speed hit no longer has much force.  And I see
> a couple of arguments in favor of allowing this code to trigger on
> single-column FKs.  First, it should work well even when pg_statistic data
> is missing or out of date, which would be an improvement; and second, we
> are likely not going to get much beta testing of the behavior if we
> restrict it to multi-col FKs.  So I think we should ship it like this for
> beta even if we end up adding a filter against single-column FKs for
> release.

OK

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services