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.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
