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

On 9 May 2016 at 00:24, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:

> Hi,
>
> Attached is a minor revision of the patch posted by David a few days ago,
> rebased on the current master (which already includes 68d704 fixing the
> segfault that started this thread).
>
> The modifications are fairly small:
>
> * The 'possibleRef' flag is renamed to 'use_for_estimation' which I think
> better describes the purpose of the flag.
>
> * The mark_useful_foreign_keys now skips foreign keys on a single column,
> as those are not useful for the patch at all. This should further reduce
> performance impact in the common case.
>

Good, thanks.

> * I've slightly reworded some of the comments, hopefully for the better.

> But now the bad news - while reviewing the David's fixup patch, I've
> noticed this comment
>
> /* XXX do we need to add entries for the append_rel_list here? */
>
> and I've realized that the estimation does not quite work with partitioned
> tables, as it only checks foreign keys on the parent tables, but the child
> tables may not use the foreign keys at all, or even use different foreign
> keys (a bit bizzare, but possible).
>
> Obviously the simplest solution would be to simply stop considering
> foreign keys with partitioned tables - that seems a bit unfortunate, but
> AFAIK we don't inspect child tables during planning anyway, and in the
> worst case we fall back to the current estimate.
>
> It might be possible to improve this by checking that all child tables
> have a matching foreign key (referencing the same table), which would allow
> us to handle the case with one side partitioned. And maybe some other (more
> complex) cases, like equi-partitioned tables. But all of this would require
> a fair amount of new code, far more than we should commit in beta mode.
>
>
> To summarize all of this, I think David's patch marking usable foreign
> keys greatly reduces the overhead compared to the committed version. The
> 'skip 1-column FKs' significantly reduces (or even eliminates) the overhead
> for the common case where only few FKs use multiple columns.
>
> Not handling the inheritance properly is clearly a serious oversight,
> though. Tom is clearly right that this got committed a bit too early.
>
> If the conclusion is that the current performance impact is still not
> acceptable, or that we need a better solution to the inheritance issue than
> simply disabling the FK estimates, then I think reverting the patch is the
> only possible solution at this point.

I disagree.

The purpose of the patch is to improve planning in simple and important
common cases. It does that. The fact that it does not cover all cases is
understood and we will make more progress in future releases. We don't
handle the inheritance case well in many ways is not this patches' fault,
or problem.

There's no point putting years of effort into parallel query if we can't
work out when to use it sensibly. This patch makes major gains in that area.

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services