Hi Robert, On Thu, Mar 12, 2026 at 10:16 AM Robert Haas <[email protected]> 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
