On Tue, Oct 27, 2026 at 9:33 PM Ayoub Kazar <[email protected]> wrote: > Now i'm not sure whether this is fine to do or not, especially given > that this kind of transformations has never been done in any other place > and this comes at an intermediate state of planning. At this point any > issue with pg_plan_advice is gone, because the patch has a more > standard-ish planning state than before. > > what do you think ?
Well, I think that you've submitted multiple versions of this patch now that failed tests or had easy-to-find defects. I suggest putting considerably more effort into this patch before posting each new version. That should certainly include making the tests pass, but also deeper thinking about what the patch is doing and how it's doing it, and LLM-driven review as well. With respect to pg_plan_advice specifically, I tried to design the module in a way where most planner features wouldn't need to make updates to it, but planner changes that create entirely new plan shapes likely will have to extend pg_plan_advice as well. Or, at the very least, find some way to exclude whatever they do from its scope, so that test_plan_advice doesn't visibly break. One wrinkle in this particular case is that set operation planning isn't currently covered by pg_plan_advice, because we have no hooks with which to enforce advice in such locations. That would be good to fix, regardless of this patch. Another wrinkle is that this patch uses set operation constructs in places where the current planner doesn't, which will complicate pg_plan_advice's attempts to understand the resulting plan. That might not be the first thing to worry about here. Making sure that everything works correctly in all the straightforward cases is probably priority 1, and using something like sqlsmith to aid testing will probably help. >From an design point of view, explosions in memory consumption and planning time are worth a lot of careful thought. It's probably not that difficult to just impose some kind of arbitrary cutoff, but that tends not to be a very nice solution. For example, if you say, well, we'll only consider this optimization when the number of "OR" arms is 4 or less, then somebody might see an abrupt plan quality regression when they go from 4 "OR" arms to 5. It's worth asking whether there's some way to degrade gracefully rather than having a sharp edge. Zsolt's point about what you've excluded from the patch is also worth a lot of thought. A patch that excludes cases unnecessarily because they seemed hard is unlikely to be accepted. On the other hand, a patch that tries too hard to accept cases that aren't a natural fit for what the patch does is also unlikely to be accepted. You want to hit the sweet spot in the middle, where you're supporting the things that you reasonably can, and punting the things you can't reasonably support with clear explanations as to why not. There's an old adage that the first 90% of the project accounts for 90% of the work, and the last 10% of the project accounts for the other 90% of the work. In other words, people tend to underestimate what percentage of the necessary effort they've already invested. It's easy to feel good when things start working, and to underestimate how much polishing remains before you have something that can actually be shipped. Reading the thread and not the patch, my estimate of what part of the required work you've done is around 1%. I'm not saying that to be mean: I'm trying to help you understand what the magnitude of this project is likely to be, so you can make an informed decision about how to proceed. I'd love to have this feature, but it's not going to be easy to get from here to there. -- Robert Haas Databricks
