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

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.

* 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.