Re: pg9.6 segfault using simple query (related to use fk for join estimates)

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

We need to make some decisions here PDQ. I haven't had time to look
at this issue in any technical detail yet. Simon, anyone else, please
weigh in.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company