Hi,
On 05/02/2016 09:18 PM, Robert Haas wrote:
> On Sat, Apr 30, 2016 at 1:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Julien Rouhaud <julien.rouhaud@dalibo.com> writes:
>>> On 29/04/2016 18:05, Tom Lane wrote:
>>>> Julien Rouhaud <julien.rouhaud@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