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

Hi,

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.1465069446@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.

OK

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

OK

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

regards

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