Re: pg_*_advice: tsv load failure, etc.

On Fri, Sep 4, 2026 at 3:40 PM Robert Haas <robertmhaas(at)gmail(dot)com> 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