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