On Fri, Sep 4, 2026 at 3:40 PM Robert Haas <[email protected]> wrote: > There are still some more things to fix here, so I'll keep working on > this next week.
Here is a new patch set. Since my last update, I have committed the previous 0001 as dc33cca1859e2d2500ee8f3c0e564561428b4fcf and the previous 0002 as 1129570f654e96d45bbc63397079171af29710b9. In this new patch set, 0001 and 0002 correspond to the previous 0003 and 0004, and improve the code and documentation in terms of how this feature interacts with GEQO. 0003 from this patch set corresponds to the previous 0005, but the scope has been expanded to fix additional problems in the same area of the code. Generally, all of these problems stem from advice enforcement (which tries to make the plan obey the advice) being out of step with advice feedback (which says whether the plan actually did obey the advice). As far as I have found so far, those things are in lock step for all of the cases that test_plan_advice exercises, or to say it differently, they're in lock step for all the kinds of advice that pg_plan_advice generates itself. But if you write your own custom advice strings, you can do things that are legal but not what the advice generator would have done, and then problems ensue. For example, you could write JOIN_ORDER((a b) c) instead of the more intuitive JOIN_ORDER(a b c); the meaning is identical either way. This patch fixes several of the worst issues; see the commit message for details. There are also some cases that it doesn't fix. I know of two such cases, one easy and one hard. The easy problem is with zero-element sublists, like JOIN_ORDER(a ()). I suspect the correct thing to do is reject this case in the advice string parser, similar to what 8c9c2e5c09aeda713640ccb26bfca0fdccc310d5 did for FOREIGN_JOIN. I have not deeply investigated this yet so maybe there's some wrinkle that needs considering, but I expect fixing this to be straightforward. The hard problem is finding #10 from Noah's original report, which is the case where you have two partitions of the same parent table that have the same name but are in different schemas, and you omit the schema name from the relation identifier. That is, you have a table, let's say foo, with partitions bar.foopart and baz.foopart. Instead of writing something like SEQ_SCAN(foo/bar.foopart) as the advice generator would do, you choose to write SEQ_SCAN(foo/foopart), making it ambiguous which child relation you're talking about. What happens right now is that advice enforcement will enforce that restriction against both children, but advice feedback will think that doesn't affect either one, so if you look at the advice feedback with EXPLAIN or print it out via pg_plan_advice.feedback_warnings, you'll see "matched, failed" instead of just "matched". Obviously, this kind of sucks. Of course, the blast radius is limited by the fact that almost nobody names their partitions this way, but theoretically they could and nobody likes features that work most of the time. Unfortunately, it is not at all straightforward to fix this. Pretty much everything the advice feedback code starts by mapping each relation identifier to an RTI, and giving up if the number of matches is not exactly 1. Since the whole purpose of relation identifiers is to map uniquely to relations appearing in the query, this is in general fine, but the case described in the previous paragraph breaks it. Generalizing the logic in the advice feedback code to be able to handle a relation identifier that maps to multiple RTIs looks like a bad idea. It would require massive adjustments to the code for a case that almost nobody has in real life, and the resulting code would be complicated and hard to understand and probably have corner cases where the performance is terrible. So I think something else has to give. A possible zero-order solution is to just document that advice feedback isn't guaranteed to work properly if you do this, so maybe you shouldn't. That is obviously not amazing but I think we could live with it for v19. What I'm thinking is probably better is to impose a restriction sufficient to keep this case from arising in the first place, such as: 1. Just rip out all the logic that allows the partition schema to be omitted, and require it always. This is only sad for people writing advice strings manually, since generated advice always includes the schema anyway. 2. Refuse to enforce advice if the partition schema is omitted and there's more than one partition with the same partition name. This changes the rule from "you can leave out the partition schema" to "you can leave out the partition schema when no ambiguity is thereby created," and it makes the existing behavior of the advice feedback system correct in retrospect. This seems like it would make just about nobody sad and therefore be just about perfect, except that I am not sure there's any way of implementing it that doesn't result in causing even bigger problems that the one it's trying to solve. More investigation needed. Or maybe there is another idea that is better than either of those; I need to look into this further. -- Robert Haas EDB: http://www.enterprisedb.com
v4-0002-Change-GEQO-fitness-comparisons-to-consider-disab.patch
Description: Binary data
v4-0003-pg_plan_advice-Fix-defects-in-JOIN_ORDER-advice-f.patch
Description: Binary data
v4-0001-pg_plan_advice-Document-interaction-with-GEQO.patch
Description: Binary data
