Re: pg_plan_advice

Hi Robert,

On Thu, Mar 12, 2026 at 10:16 AM Robert Haas <robertmhaas(at)gmail(dot)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 = 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 = 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

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