Hi,
On 05/02/2016 09:18 PM, Robert Haas wrote:
On Sat, Apr 30, 2016 at 1:35 PM, Tom Lane <t...@sss.pgh.pa.us> 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.
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.
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.
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.
If this gets reverted, then probably
015e88942aa50f0d419ddac00e63bb06d6e62e86 should also be reverted.
Probably. I think the code is fine / correct, but as the FK estimation
patch was the only user, there's not much point in keeping it if that
gets reverted.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
