On 04/30/2016 07:35 PM, Tom Lane wrote:
Julien Rouhaud <julien.rouh...@dalibo.com> writes:
On 29/04/2016 18:05, Tom Lane wrote:
Julien Rouhaud <julien.rouh...@dalibo.com> writes:
The segfault is caused by quals_match_foreign_key() calling get_leftop()
and get_rightop() on a ScalarArrayOpExpr node.

I'm not sure that assuming this compatibility is the right way to fix
this though.

It certainly isn't.

Agreed. New attached patch handles explicitly each node tag.

No, this is completely nuts.  The problem is that quals_match_foreign_key
is simply assuming that every clause in the list it's given is an OpExpr,
which is quite wrong.  It should just ignore non-OpExpr quals, since it
cannot do anything useful with them anyway.  There's a comment claiming
that non-OpExpr quals were already rejected:

             * Here since 'usefulquals' only contains bitmap indexes for quals
             * of type "var op var" we can safely skip checking this.

but that doesn't appear to have anything to do with current reality.

Yes, I agree - there should be a check that the node is OpExpr in quals_match_foreign_key. This is clearly a bug :-(

While this in itself is about a two-line fix, after reviewing
137805f89acb3611 I'm pretty unhappy that it got committed at all.
I think this obvious bug is a good sign that it wasn't ready.
Other unfinished aspects like invention of an undocumented GUC
don't leave a good impression either.

The GUC is meant to make testing during development easier. I haven't realized it got committed, but I assume Simon plans to remove it before the final release. Can't check right now with Simon, though, as he's is out of office this week.

In any case, I do agree the GUC should have been documented better, and we should probably put it on the TODO so that it gets removed before the actual release.

Moreover, it looks to me like this will add quite a lot of overhead,
probably far more than is justified, because
clauselist_join_selectivity is basically O(N^2) in the
relation-footprint of the current join --- and not with a real small
multiplier, either, as the functions it calls contain about four
levels of nested loops themselves. Maybe that's unmeasurable on
trivial test cases but it's going to be disastrous in large queries,
or for relations with large numbers of foreign keys.

That depends on a lot of factors, I guess. Attached are two SQL scripts that create 5 tables (f1, f2, f3, f4, f5) and then create N foreign keys on each of them. The foreign keys reference other tables, which means the loops won't terminate early (after finding the first match) when joining the first 5 tables, which makes this the worst case. And then we join different numbers of tables (2, 3, 4 or 5) and do explain analyze to measure planning time.

The first script (fk-test.sql) does joins on 2 columns, fk-test-6.sql does joins on 6 columns (so that we also know how the number of columns
affects the planning time).

Sum of planning time for 100 queries (in milliseconds) with 2 columns, where "2/on" means "join on 2 tables with enable_fk_estimates=on":

N       2/on    2/off   3/on    3/off   4/on    4/off   5/on    5/off
10      6       4       9       8       22      20      77      68
100     15      11      26      19      64      36      177     93
1000    97      84      217     133     516     233     1246    342

and comparison of the timings:

        2       3       4       5
10      155%    115%    114%    113%
100     142%    138%    177%    190%
1000    116%    163%    221%    364%

And with the 6 columns:

        2/on    2/off   3/on    3/off   4/on    4/off   5/on    5/off
10      25      7       23      20      96      82      344     297
100     21      14      65      33      233     104     735     328
1000    151     99      492     153     1603    320     4627    593

and comparison:

        2       3       4       5
10      371%    120%    117%    116%
100     149%    196%    224%    224%
1000    152%    322%    502%    780%

So yes, the overhead is clearly measurable, no doubt about that.

I think this should be reverted and pushed out to 9.7 development.
It needs a significant amount of rewriting to fix the performance
issue, and now's not the time to be doing that.

There are probably a few reasonably simple things we could do - e.g. ignore foreign keys with just a single column, as the primary goal of the patch is improving estimates with multi-column foreign keys. I believe that covers a vast majority of foreign keys in the wild.

If that's deemed insufficient, we'll have to resort to a more complex improvement, perhaps something akin to the cache proposed in in the unijoin patch. But if that's required, that's 9.7 material.


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

Attachment: fk-test-6.sql
Description: application/sql

Attachment: fk-test.sql
Description: application/sql

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to