Robert Haas <robertmh...@gmail.com> writes: > FYI, I spoke to Tom Lane about this at PGCon and suggested that he > look at the proposed patch as I requested in > https://www.postgresql.org/message-id/CA+TgmobPqrAVXOBMHTcpDq8hX7gCzcVhoUvC8s9V=d09+bt...@mail.gmail.com > and see whether that would address his concerns, and he said that he > would do that. It may, however, have slipped his mind.
> My opinion is that something needs to be done about this patch. It > needs to be improved or reverted. Improved would be ideal in my mind, > but reverted is an outcome I can live with. I've taken a look at fkest_fixes_v2.patch. I do not like mark_useful_foreign_keys one bit: it adds *more* overhead to *every* query, whether or not there's anything to be saved. Also, adding a flag to the ForeignKeyOptInfo entries doesn't save having to iterate over the irrelevant entries. We should arrange for them not to be in the list in the first place. The direction I would go in to get rid of irrelevant FKs is more like this: * 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). * 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. But more generally, this entire approach does very little to improve the problem of nested loops leading to probable poor big-O behavior. I previously complained that the patch adds code that is executed > * at least once per join relation created (hence, significantly more than > the number of rels in the query; see also get_joinrel_parampathinfo) > * for each inner relation in the initial input joinrel pair > * for each outer relation in the initial joinrel pair > * for each foreign key constraint on this inner and outer rel > * for each key column in that FK > * for each join qual for the current input joinrel pair > * for each member of the relevant EquivalenceClass and only the fourth of those seven nested loops is significantly shortened by this patch. It's still about as brute-force as can be. I think that fkest_fixes_v2.patch really only makes things significantly better for cases where a table in the query has a huge number of FKs leading to tables not in the query, and TBH I think that's an artificial test condition that will seldom have much to do with real-world performance. It's also pretty bad that "for each foreign key constraint on this inner and outer rel" means "for each FK leading out of either the inner or outer rel, whether or not it leads to the other side". The existing code has to do that in order to handle cases where the particular representative clause generated from an EC doesn't match the FK but another clause could have. I still think that some more careful thought about relating FKs to ECs could result in elimination of a couple of these looping levels altogether. This is certainly all attackable, and I'm even willing to help work on it, but it's going to lead to a rather larger patch than we usually like to apply in beta. And there are still all the non-planning-speed cleanup issues to be dealt with. fkest_fixes_v2.patch addresses some of them but I still feel that a lot of work is needed on the comments. 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. We need to decide whether we want to put a significant amount of time into this patch over the next couple of weeks, to the exclusion of other things, or revert it until the next devel cycle opens. Since we're not exactly just sitting around twiddling our thumbs, I'm hesitant to commit the required amount of time to pursue the first course. But we've commissioned the RMT to make these sorts of decisions, so it's in their lap. regards, tom lane -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers