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>
 <CAOYmi+k4AyWCQHK=XVF99KVDuFkqxcADao61OWGLxu0nRYMONQ@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>
 <CAK98qZ1zWzRB0ABG7ULzTeWKRR5C7-FxLqM-6v8wQDiFM+DNAg@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>
 <CAKFQuwYxTMx1e1dvVLd5vs8CO2Xe__YRCW0m4v2UErG=5Lu7kQ@mail.gmail.com>
 <CA+TgmoaPH3rOmH1zw8s10Az3TQKRbDnF3dKYZCAFvx_DgFV5_Q@mail.gmail.com>
 <CAKFQuwZ7CRbvmJ60EjvEUJP8X9hWVCHJz8yJZ0-xRpYwL=fugA@mail.gmail.com>
 <CA+TgmoYsG2pAOG4YYWwnGsaL+QfOZopi_m+f_hZqBZ3meNEkjg@mail.gmail.com>
 <CAKFQuwY4i6xXCRHd=ccrFz0sQOfiWRxhobiBrzyK0rzu1-Y33Q@mail.gmail.com>
 <CA+Tgmobbj_TaCsYmr1grJBTRKaFaxFfotXn1T6LSXs9GQ8_Kyw@mail.gmail.com>
 <CA+TgmoZzM2i+p-Rxdphs4qx7sshn-kzxF91ASQ5duOo0dFRXLQ@mail.gmail.com>
In-Reply-To: 
 <CA+TgmoZzM2i+p-Rxdphs4qx7sshn-kzxF91ASQ5duOo0dFRXLQ@mail.gmail.com>
From: Alexandra Wang <alexandra.wang.oss@gmail.com>
Date: Thu, 12 Mar 2026 16:45:49 -0700
Message-ID: 
 <CAK98qZ3bySpnVTUKiDRZQ62mfJ=v4JsZJfAKaahjBYmWjr3fwQ@mail.gmail.com>
Subject: Re: pg_plan_advice
To: Robert Haas <robertmhaas@gmail.com>
Cc: "David G. Johnston" <david.g.johnston@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: multipart/alternative; boundary="00000000000054bb30064cdc5c4e"
Archived-At: 
 <https://www.postgresql.org/message-id/CAK98qZ3bySpnVTUKiDRZQ62mfJ%3Dv4JsZJfAKaahjBYmWjr3fwQ%40mail.gmail.com>
Precedence: bulk

--00000000000054bb30064cdc5c4e
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Robert,

On Thu, Mar 12, 2026 at 10:16=E2=80=AFAM Robert Haas <robertmhaas@gmail.com=
> wrote:
> I'm still hoping to get some more feedback on the remaining patches,
> which are much smaller and conceptually simpler. While there is no
> time to redesign them at this point in the release cycle, there is
> still the opportunity to fix bugs, or decide that they're too
> half-baked to ship. So here is v20 with just those patches. Of course,
> post-commit review of the main patch is also very welcome.

Thanks for the patches!

I've run the meson tests for all three modules, and they all pass on
my laptop.

For the make tests, I had to add

+EXTRA_INSTALL =3D contrib/pg_plan_advice

in contrib/pg_collect_advice/Makefile in order for "make check" for
that module to run.

I don't really have a sense of how others feel about including these
modules, so I can't speak to that. Personally, though, I very much
like the test_plan_advice module because it gives me peace of mind,
and I feel it should accompany the already committed pg_plan_advice
module.

I reviewed the code and have a few minor comments:

0001:
The "make check" issue mentioned above.

0002:
Looks good to me.

0003:
There is a typo in the commit message:
"pg_stash.advice_stash" should be "pg_stash_advice.stash_name".

> stash->pgsa_stash_id =3D pgsa_state->next_stash_id++;

I doubt there will be more than 2 billion stashes in the system, but
if in any case we reach that number, we don't handle int overflow.
Should we set a limit on how many stashes can be stored?

Nit:
find_defelem_by_defname() is defined in all three modules, and also in
pg_plan_advice. Knowing it is very small, would it make sense to
extern the one in pg_plan_advice and reuse it?

Best,
Alex

--=20
Alexandra Wang
EDB: https://www.enterprisedb.com

--00000000000054bb30064cdc5c4e
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div>Hi=C2=A0Robert,<br><br>On Thu, Mar 12, 2026 at 10:16=
=E2=80=AFAM Robert Haas &lt;<a href=3D"mailto:robertmhaas@gmail.com">robert=
mhaas@gmail.com</a>&gt; wrote:<br>&gt; I&#39;m still hoping to get some mor=
e feedback on the remaining patches,<br>&gt; which are much smaller and con=
ceptually simpler. While there is no<br>&gt; time to redesign them at this =
point in the release cycle, there is<br>&gt; still the opportunity to fix b=
ugs, or decide that they&#39;re too<br>&gt; half-baked to ship. So here is =
v20 with just those patches. Of course,<br>&gt; post-commit review of the m=
ain patch is also very welcome.<br><br>Thanks for the patches!<br><br>I&#39=
;ve run the meson tests for all three modules, and they all pass on<br>my l=
aptop.=C2=A0<br><br>For the make tests, I had to add=C2=A0<br><br>+EXTRA_IN=
STALL =3D contrib/pg_plan_advice<br><br>in contrib/pg_collect_advice/Makefi=
le in order for &quot;make check&quot; for<br>that module to run.<br><br>I =
don&#39;t really have a sense of how others feel about including these<br>m=
odules, so I can&#39;t speak to that. Personally, though, I very much<br>li=
ke the test_plan_advice module because it gives me peace of mind,<br>and I =
feel it should accompany=C2=A0the already committed pg_plan_advice<br>modul=
e.<br><br>I reviewed=C2=A0the=C2=A0code and have a few minor comments:=C2=
=A0<br><br>0001:<br>The &quot;make check&quot; issue mentioned above.<br><b=
r>0002:<br>Looks good to me.<br><br>0003:<br>There is a typo in the commit =
message:<br>&quot;pg_stash.advice_stash&quot; should be &quot;pg_stash_advi=
ce.stash_name&quot;.<br><br>&gt; stash-&gt;pgsa_stash_id =3D pgsa_state-&gt=
;next_stash_id++;<br><br>I doubt there will be more than 2 billion stashes =
in the system, but<br>if in any case we reach that number, we don&#39;t han=
dle int overflow.<br>Should we set a limit on how many stashes can be store=
d?<br><br>Nit:<br>find_defelem_by_defname() is defined in all three modules=
, and also in<br>pg_plan_advice. Knowing it is very small, would it make se=
nse to<br>extern the one in pg_plan_advice and reuse it?<br><br>Best,</div>=
<div>Alex<br></div><div><br></div><span class=3D"gmail_signature_prefix">--=
 </span><br><div dir=3D"ltr" class=3D"gmail_signature"><div dir=3D"ltr"><fo=
nt face=3D"monospace">Alexandra Wang</font><div><font face=3D"monospace">ED=
B: <a href=3D"https://www.enterprisedb.com" target=3D"_blank">https://www.e=
nterprisedb.com</a></font></div></div></div></div>

--00000000000054bb30064cdc5c4e--