MIME-Version: 1.0
References: 
 <CA+TgmoZ-Jh1T6QyWoCODMVQdhTUPYkaZjWztzP1En4=ZHoKPzw@mail.gmail.com>
 <CAKZiRmxtJAFG7e1+Vs9B8ngON=AOzJbuws+1ZeH4LsbJh5AzoQ@mail.gmail.com>
 <CA+TgmoY9Ne_Sh10u6LSPc3wvOQPLp3kF9nBp3nqJEG2JGF2QiA@mail.gmail.com>
 <CA+Tgmoa57S6mP=aTOXH2-gDAL4TMO1WbGgrHSg0s6J4zUH=04g@mail.gmail.com>
 <DEB1SAGVDX7S.JMV96X4R0RCS@gmail.com>
 <CA+Tgmoaf__2B0BUL+vrg28P+3buX=Ti-kybqkHiLTtFrrCfzuA@mail.gmail.com>
 <CA+TgmoYpcLNOuypOTdgCSLW7FuA=t6BtB3meTARHX2-Dj_81xQ@mail.gmail.com>
 <DEFNP3WX7BLX.1X7DS1P9X6JIF@nmfay.com>
 <CA+TgmoZjv9OyFu1Gkt78w0vWEti8S33w8trYHmErf-GMmGSi=w@mail.gmail.com>
 <DELPC4B56Y5A.3OBZ3WCH3T4VX@nmfay.com>
 <CA+TgmoaOSBQD9Ux4eG40w723ZN=c0J7p-+oX4+J8urUeyLMo5w@mail.gmail.com>
 <CAOYmi+=g+MMoOpWkk2weXWKJcKH0eKey8gKHHdH0dF4Tiawrhw@mail.gmail.com>
 <CA+TgmobwaT=PXPDDrgDup+jA8KHBbkxigtziD-zNzAKKkQYVgQ@mail.gmail.com>
 <CAOYmi+mOmEW=amDRQMfw6-Fb3ZmDEQFaJzwk8Bc8W8DzaP85XQ@mail.gmail.com>
 <CA+TgmoaX2AMW4cdFM3OngBJxmxpkdmzF33R7-CWhvRLfucbFMg@mail.gmail.com>
 <CA+TgmoZ0x3ym_oueXRWzbM_=6ucKoPZVGj3rRMLBDC_FnetXDw@mail.gmail.com>
 <CAP53Pkycc=7N2bLzVT3x+qE1JamvRZWev5tFjdLJ1+-AV3Di+Q@mail.gmail.com>
 <CA+TgmoaKhuD91RnazbRyGkmP7--JdNq8oNDC3UcgTZSWbMxC7w@mail.gmail.com>
 <CAP53Pkw5-wMEeDJXFmqo_RTyL_spzCXb7HHKrbSnQqokVoZcNQ@mail.gmail.com>
 <CA+Tgmob-69bzbdi3U_QtebqAf6u1y8js=5=oNK639csVe1VbhA@mail.gmail.com>
 <CA+TgmoaZMOikxK=LqS+Jn+835h9S139JLGk-3LyETVXw5W5j=w@mail.gmail.com>
 <1136161.1769654478@sss.pgh.pa.us>
 <CAP53PkwZ1ZTMARKg6iEfAw9qzBhkjBitj-9gr_Jvy7k2AwGgWA@mail.gmail.com>
 <CAMbWs4--NuEUFE_xTo991TRXaZryE29jarJPDnVxoaQOYdt7tA@mail.gmail.com>
 <CA+TgmobzR+XMGbRosVPbjHbSo4+cgJn=qZK6w05aF1sbj=C+9Q@mail.gmail.com>
 <CA+TgmoawzvCoZAwFS85tE5+c8vBkqgcS8ZstQ_ohjXQ9wGT9sw@mail.gmail.com>
 <CA+TgmoYS4ZCVAF2jTce=bMP0Oq_db_srocR4cZyO0OBp9oUoGg@mail.gmail.com>
 <CAK98qZ2RzbgCHrSg4zLkvpzyBam_X6te-KF8w1+_vON9BAVMEw@mail.gmail.com>
 <CA+TgmoaCdsuvNn6T6SfQ_0YD2Hh2+hgTXh9fTGHQhPg1zvy2rQ@mail.gmail.com>
 <CA+Tgmob7ozJAs5SU6bD2RfAt4w_AmsMGz-aaVA6WeLXHkBypOg@mail.gmail.com>
 <CAK98qZ1J42RoAsHnYWGPPmHziZmzmqE=Lp_O6WJ-9aKK2qjikA@mail.gmail.com>
 <CA+TgmoYjcBA6dw3nwiyfDzPXTCrxTZPXDMrc2TrDJcL1cPK6iA@mail.gmail.com>
 <CA+TgmoYru-vxoTKfwjQby30r2OkTXfb18Km_=VLs6qk8Akr0-g@mail.gmail.com>
 <CA+Tgmoau7yJtvbeH-0kPt1Q=Gt_ezRdgM35Q1=LT665U_86Etg@mail.gmail.com>
 <1299934.1773938807@sss.pgh.pa.us>
 <CA+TgmobOLrMn5jEinWNPL5SrDH1DPpo3a4j+S6-4yhsZwWgzLg@mail.gmail.com>
 <CA+TgmoZUN8FT1Ah=m6Uis5bHa4FUa+_hMDWtcABG17toEfpiUg@mail.gmail.com>
In-Reply-To: 
 <CA+TgmoZUN8FT1Ah=m6Uis5bHa4FUa+_hMDWtcABG17toEfpiUg@mail.gmail.com>
From: Robert Haas <robertmhaas@gmail.com>
Date: Sat, 21 Mar 2026 09:13:59 -0400
Message-ID: 
 <CA+TgmoYh2-kM+tscOz=jVYq9Tf4SRPVqzPojs3KLZcW6E9m1BQ@mail.gmail.com>
Subject: Re: pg_plan_advice
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CA%2BTgmoYh2-kM%2BtscOz%3DjVYq9Tf4SRPVqzPojs3KLZcW6E9m1BQ%40mail.gmail.com>
Precedence: bulk

On Thu, Mar 19, 2026 at 4:38=E2=80=AFPM Robert Haas <robertmhaas@gmail.com>=
 wrote:
> schnauzer's got a failure that looks like this:
>
> +WARNING:  supplied plan advice was not enforced
> +DETAIL:  advice SEQ_SCAN(pg_trigger@exists_1) feedback is "matched, fail=
ed"
> +advice NO_GATHER(pg_trigger@exists_1) feedback is "matched, failed"
>
> And an older run on skink has a failure that looks like this:
>
> +WARNING:  supplied plan advice was not enforced
> +DETAIL:  advice SEQ_SCAN(pg_trigger@exists_6) feedback is "matched, fail=
ed"
> +advice NO_GATHER(pg_trigger@exists_6) feedback is "matched, failed"

I spent a bunch of time looking into this. I don't have a definitive
answer to the question of what to do about it yet, but I wanted to
write down some initial thoughts.

First, I think that I broke fix_alternative_subplan when I added
disabled_nodes. Before, when disabling things just affected the cost,
the logic here would have taken what was disabled into account in
picking between alternatives. Now it doesn't, because I didn't add a
disabled_nodes field to SubPlan. You can see that it breaking with
this test case:

CREATE TABLE t1 (a int);
CREATE TABLE t2 (a int);
CREATE INDEX ON t2(a);
INSERT INTO t1 SELECT generate_series(1, 1000);
INSERT INTO t2 SELECT generate_series(1, 100000);
ANALYZE;

EXPLAIN (VERBOSE, COSTS ON)
SELECT * FROM t1
WHERE EXISTS (SELECT 1 FROM t2 WHERE t2.a =3D t1.a) OR t1.a < 0;

SET enable_seqscan =3D off;
SET enable_indexonlyscan =3D off;

EXPLAIN (VERBOSE, COSTS ON)
SELECT * FROM t1
WHERE EXISTS (SELECT 1 FROM t2 WHERE t2.a =3D t1.a) OR t1.a < 0;

With the currently-committed code, you get a hashed SubPlan both
times, and it's just disabled in the second case. But there's a
perfectly good non-hashed variant that is not disabled: scan t2 using
a parameterized Index Scan. So that sucks. I have a small patch to fix
this, which I'll post later. I don't think we can or should do
anything about this in released branches, but we should fix it in
master regardless of what happens to pg_plan_advice or
test_plan_advice.

Second, in terms of actually fixing the problem, I think the issue
here is that the scope I chose for pg_plan_advice doesn't quite fit
the goal of making test_plan_advice the canonical way of testing this
code. In order to keep this project manageable in size, I decided
that, for the first version, pg_plan_advice wasn't going to try to
control anything above the level of scan/join planning, so for example
we don't care what kind of aggregation the planner chooses. That
should be OK for test_plan_advice, because test_plan_advice works by
checking if all the advice applied cleanly, and since we didn't emit
any advice about the aggregation method in the first place, there
can't be any problem with applying it later. In other words, if the
aggregation method chosen does flip between consecutive planning
cycles, it should not cause a test_plan_advice test failure. This same
general principle applies to a bunch of other cases too, like set
operation planning: if there's more than one way to do it, the planner
can change its mind and test_plan_advice should not care. However,
there's an important exception: if something changes above the level
of scan/join planning that affects what rels are involved in scan/join
planning, then a plan change will cause a test_plan_advice failure.

The failures above are of that type: the way the AlternativeSubPlan
machinery works is that the query gets copied and replanned, and each
plan has a separate plan name. So the final plan has either
pg_trigger@exists_5 or pg_trigger@exists_6 in it, but never both.
There's one other case that I think is similar, which is the
MinMaxAggPath stuff: if we choose the MinMaxAggPath, then the final
plan will have something like t1@minmax_1 where it otherwise would
have had just t1, or perhaps t1@minmax_1 instead of t1@something_else.
These cases have something in common, which is that they are the only
cases where we make a new PlannerInfo to try replanning part of the
query in a second way. That's the pattern that causes breakage here. I
would be remiss if I did not mention that Jakub Wartak was poking at
me about the MinMaxAggPath case a while back, but I dismissed it as
out of scope, which was accurate, but that's because I didn't think at
the time that it would destabilize test_plan_advice. Now, I think it
could, although I don't think we have seen any such failures in the
buildfarm yet. Perhaps a concurrent statistics change is more likely
to flip the hashed/non-hashed SubPlan decision than the choice of
whether to use MinMaxAggPath.

One approach that I considered here is to try to unify the "sibling"
relations. If the final plan is bound to contain either
pg_trigger@exists_5 or pg_trigger@exists_6, maybe the advice shouldn't
actually think there are two separate things. Maybe instead of
subqueries exists_1 ... exists_6 we should end up with subqueries
exists_1 ... exists_3, with each name used twice. That amounts to
deciding that the patch to give each subplan a unique name got it
wrong. While this idea has some appeal, ultimately I think it's a
loser, because addressing the problem this way wouldn't actually fix
the test_plan_advice instability we're currently seeing, or at least
not necessarily. For example, in the test case earlier in this email,
INDEX_SCAN(t2@whatever) can only be applied if the non-hashed subplan
is chosen, because we won't consider a plan index scan to even be a
possibility for a full table scan. An index-only scan is considered,
but not a plain index scan. This means that even if we flattened the
rels in each pair of siblings together and called each pair by the
same name, we could still see failures to apply advice cleanly.
Moreover, that's assuming that optimizations like self-join
elimination, left join removal, and partition pruning always apply in
the same way to both copies, which doesn't sound like a safe
assumption at all.

A second approach is to try to control the initial conditions of the
two planning cycles better. Possibly just running the tests serially
instead of in parallel would get that done, but that seems too slow to
consider and, even if we did it anyway, I'm not all that sure that an
autoanalyze or autovacuum in the background couldn't mess us up. Or,
alternatively, if we could keep the second planning cycle from seeing
different statistics than the first, I think that would do it, but I
don't think there's a feasible way of doing that.

So I'm left with the idea that to get test_plan_advice to be fully
stable on these slower machines, it will probably be necessary to make
it control which AlternativeSubPlan is chosen and whether a
MinMaxAggPath is chosen or not. I have some ideas about how to
accomplish that in a reasonably elegant fashion without adding too
much new machinery, but I need to spend some more time validating
those ideas before committing to a precise course of action. More
soon.

--=20
Robert Haas
EDB: http://www.enterprisedb.com