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>
 <CA+TgmoYh2-kM+tscOz=jVYq9Tf4SRPVqzPojs3KLZcW6E9m1BQ@mail.gmail.com>
 <CA+TgmoaK=4w7-qknUo3QhUJ53pXZq=c=KgZmRyD+k7ytqfmgSg@mail.gmail.com>
In-Reply-To: 
 <CA+TgmoaK=4w7-qknUo3QhUJ53pXZq=c=KgZmRyD+k7ytqfmgSg@mail.gmail.com>
From: Lukas Fittl <lukas@fittl.com>
Date: Wed, 25 Mar 2026 16:59:06 -0700
Message-ID: 
 <CAP53Pkz3DSFaaowYvbO5LULf3NhydD_UhHkighfWf6_pwxiqUw@mail.gmail.com>
Subject: Re: pg_plan_advice
To: Robert Haas <robertmhaas@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>,
	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/CAP53Pkz3DSFaaowYvbO5LULf3NhydD_UhHkighfWf6_pwxiqUw%40mail.gmail.com>
Precedence: bulk

Hi Robert,

On Tue, Mar 24, 2026 at 2:10=E2=80=AFPM Robert Haas <robertmhaas@gmail.com>=
 wrote:
>
> On Sat, Mar 21, 2026 at 9:13=E2=80=AFAM Robert Haas <robertmhaas@gmail.co=
m> wrote:
> > 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.
>
> Here is v22. There are four new patches.
>
> 0001 adds a disabled_nodes fields to SubPlan, to fix the bug that I
> identified in the email to which this is a reply.

This looks good, and is consistent with how it would have worked
before the introduction of disabled nodes (since costs would have just
been very high and thus discourage a subplan with many disabled
nodes).

Only nit is that the commit hash reference in the commit message
doesn't seem right, I think you probably meant
e22253467942fdb100087787c3e1e3a8620c54b2

> 0002-0004 are an attempt to fix the remaining buildfarm failures not
> already addressed (or attempted to be addressed, anyway) by other
> commits. The basic idea, implemented by 0004, is to add a
> DO_NOT_SCAN() advice tag. This advice is generated when we consider a
> MinMaxAggPath or a hashed SubPlan. In either case, all relations which
> are part of the non-selected alternative are marked DO_NOT_SCAN(),
> which works like scan type advice but disables every possible scan
> type rather than still allowing exactly one of them. Unless I've
> missed something, this should be sufficient to make pg_plan_advice
> stabilize which of two alternative SubPlans we pick and whether or not
> a min/max aggregate is chosen. 0002 does some preliminary refactoring
> to provide a more centralized way of tracking per-PlannerInfo details
> within pg_plan_advice. 0003 makes the necessary change to
> src/backend/optimizer, which consists of adding an alternative_root
> field to each PlannerInfo and setting it appropriately. 0004 then
> updates pg_plan_advice to implement DO_NOT_SCAN().

For 0002:

I think that overall looks like a good refactoring, with two minor notes:

> diff --git a/contrib/pg_plan_advice/pgpa_planner.c b/contrib/pg_plan_advi=
ce/pgpa_planner.c
> index fee88904760..70139ff42be 100644
> --- a/contrib/pg_plan_advice/pgpa_planner.c
> +++ b/contrib/pg_plan_advice/pgpa_planner.c
> ...
> @@ -2017,34 +1949,64 @@ pgpa_planner_feedback_warning(List *feedback)
>                  errdetail("%s", detailbuf.data));
>  }
>
> -#ifdef USE_ASSERT_CHECKING
> -
>  /*
> - * Fast hash function for a key consisting of an RTI and plan name.
> + * Get or create the pgpa_planner_info for the subroot with the given
> + * plan_name.
>  */
> -static uint32
> -pgpa_ri_checker_hash_key(pgpa_ri_checker_key key)
> +static pgpa_planner_info *
> +pgpa_planner_get_proot(pgpa_planner_state *pps, PlannerInfo *root)
>  {

I'd word that as "Get or create the pgpa_planner_info for the given
PlannerInfo and its associated plan_name", since you're not passing a
plan_name as the argument.

> @@ -2053,19 +2015,34 @@ pgpa_ri_checker_save(pgpa_planner_state *pps, Pla=
nnerInfo *root,
>                      RelOptInfo *rel)
> {
> ...
> +   if (proot->rid_array_size <=3D rel->relid)
> +   {
> +           int                     new_size =3D Max(proot->rid_array_siz=
e, 8);
> +
> +           while (new_size < rel->relid)
> +                   new_size *=3D 2;

This could use pg_nextpower2_32 on the rel->relid instead of the
manual while loop.

---

For 0003:

I wonder if "original_root" wouldn't be more correct here as a name
(instead of "alternative_root"), since if I follow the implementation
correctly, you are adding a pointer on each alternative root, back to
the original root that the alternative was copied from.

I also wonder if maybe we should be more narrow in what we keep here.
It seems 0004 mainly needs the original plan name, so maybe its better
if we just keep that for targeting purposes, vs a full pointer to the
PlannerInfo. The planner makes an effort to zap unused subplans at the
end of set_plan_references, and I think this new field would then be
the only pointer to those unused subplans. If we decided to add an
early free there at some point (instead of just making them a NULL
entry in the list), that'd break pg_plan_advice.

---

For 0004:

> +    /*
> +     * If the corresponding PlannerInfo has an alternative_root, then th=
is is
> +     * the plan name from that PlannerInfo; otherwise, it is the same as
> +     * plan_name.
> +     *
> +     * is_alternative_plan is set to true for every pgpa_planner_info th=
at
> +     * shares an alternative_plan_name with at least one other, and to f=
alse
> +     * otherwise.
> +     */
> +    char       *alternative_plan_name;
> +    bool        is_alternative_plan;

Per the earlier note, I think using "original_plan_name" would make
more sense here, because it'll be the name the alternatives are based
on. I also think "has_alternative_plan" is more clear for the boolean,
since it'll be set on the info for the original info as well, if I
understand correctly.

Otherwise 0004 looks like a reasonable compromise for now. I feel like
we can find better ways of doing this over time, and there are parts
I'm not excited about (e.g. the targeting feels a bit brittle when it
comes to anything that'd cause generated alternative subplan names to
change), but I think it works for now.

> 0005 is the pg_collect_advice module from previous versions of the
> patch set. The main change here is that I completely rewrote the TAP
> test, which previously was running the entire regression test suite
> yet another time. That's been replaced with something that is much
> faster and much better targeted at properly testing the shared advice
> collector. Aside from that, I added one more check for
> InvalidDsaPointer where the code was previously lacking one.