While this thread was effectively superseded by the 'new design' thread [1], I'd like to address a few points raised here, as they are relevant for the new design (at least I believe so).

[1] https://www.postgresql.org/message-id/31041.1465069...@sss.pgh.pa.us

On 06/04/2016 08:15 PM, Tom Lane wrote:

* Make RelationGetFKeyList cache a list of ForeignKeyOptInfo structs,
not just constraint OIDs.  It's insane that the relcache scans
pg_constraint to collect those OIDs and then the planner re-reads all
those same rows on every planning cycle.  Allow the relcache to return
a pointer to the list in-cache, and require the planner to copy that
data before making any more cache requests.  The planner would run
through the list, skipping single-key entries and entries leading to
irrelevant tables, and copy out just the items that are useful for
the current query (probably adding query-specific table RTE indexes
at the same time).

That seems like a fairly straightforward change, and I'm not opposed to doing that. However RelationGetFKeyList is basically a modified copy of RelationGetIndexList, so it shares the same general behavior, including caching of OIDs and then constructing IndexOptInfo objects on each get_relation_info() call. Why is it 'insane' for foreign keys but not for indexes? Or what am I missing?

* Personally I'd probably handle the "ignore irrelevant tables" test
with a list_member_oid test on a list of relation OIDs, not a
hashtable. It's unlikely that there are enough tables in the query to
justify building a hashtable.


* All of the above should happen only if the query contains multiple
tables; there is no reason to expend even one cycle on loading FK data
in a simple single-table query.


... snip the part about nested loops (new design thread) ...

Also, there are serious bugs remaining, even without considering planning
speed.  An example I just noticed is that quals_match_foreign_key actually
fails entirely to ensure that it's found a match to the FK, because there
is no check on whether foreignrel has anything to do with the FK.  That
is, this bit:

         * We make no attempt in checking that this foreign key actually
         * references 'foreignrel', the reasoning here is that we may be able
         * to match the foreign key to an eclass member Var of a RestrictInfo
         * that's in qualslist, this Var may belong to some other relation.

would be okay if you checked after identifying a matching eclass member
that it belonged to the FK's referenced table, but AFAICS there is no such
check anywhere.

Perhaps I'm missing something, but I thought this is checked by these conditions in quals_match_foreign_key():

1) with ECs (line 3990)

        if (foreignrel->relid == var->varno &&
                fkinfo->confkeys[i] == var->varattno)
                foundvarmask |= 1;

2) without ECs (line 4019)

        if ((foreignrel->relid == leftvar->varno) &&
                (fkrel->relid == rightvar->varno) &&
                (fkinfo->confkeys[i] == leftvar->varattno) &&
                (fkinfo->conkeys[i] == rightvar->varattno))
        else if ((foreignrel->relid == rightvar->varno) &&
                         (fkrel->relid == leftvar->varno) &&
                         (fkinfo->confkeys[i] == rightvar->varattno) &&
                         (fkinfo->conkeys[i] == leftvar->varattno))

Or does that fail for some reason?


Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to