On Thu, Mar 12, 2026 at 7:46 PM Alexandra Wang <[email protected]> wrote: > For the make tests, I had to add > > +EXTRA_INSTALL = contrib/pg_plan_advice
Thanks, will fix. > There is a typo in the commit message: > "pg_stash.advice_stash" should be "pg_stash_advice.stash_name". Thanks, will fix. > 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? I think the thing to do here is just change the stash ID to a 64-bit value. I'll do that in the next version. We assume in numerous places that a 64-bit integer never overflows (e.g. LSNs) which is pretty fair considering how long it would take for that to happen. > 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? I think if we want to avoid duplicating this function, we should actually put it someplace in core, e.g. expose it via defrem.h and put the code in commands/define.c. Any user of extendplan.h is likely to need a function like this, but we shouldn't put it there because there could be other use cases as well. -- Robert Haas EDB: http://www.enterprisedb.com
