Hi Robert, On Thu, Mar 26, 2026 at 10:20 AM Robert Haas <[email protected]> wrote: > > The dangling pointers are a good point; I agree that's bad. However, > > I'd be more inclined to fix it by nulling out the alternative_root > > pointers at the end of set_plan_references. I think that would just be > > the case where root->isAltSubplan[ndx] && root->isUsedSubplan[ndx]. > > The reason I'm reluctant to just store the name is that there's not an > > easy way to find a PlannerInfo by name. I originally proposed an > > "allroots" list in PlannerGlobal, but we went with subplanNames on > > Tom's suggestion. I subsequently realized that this kind of stinks for > > code that is trying to use this infrastructure for anything, for > > exactly this reason, but Tom never responded and I never pressed the > > issue. But I think we're boxing ourselves into a corner if we just > > keep storing names that can't be looked up everywhere. It doesn't > > matter for the issue before us, so maybe doing as you say here is the > > right idea just so we can move forward, but I think we're probably > > kidding ourselves a little bit. > > Here's a new version, where I've replaced alternative_root by > alternative_plan_name, serving the same function.
Great, I think that's better for now, and if we have a broader use case in the future we can always adjust this to be the full PlannerInfo. That said, reflecting on the change, I wonder if its odd that we're copying a string pointer instead of making an actual string copy. I think that's probably okay in practice? I'm still 50/50 on the naming here, since we have the alternative sub plan that has an "alternative plan name" that's not that of the alternative itself, but rather the base plan that was utilized. But I see your concern regarding the naming being confusing in terms of what the "original" or "base" would actually refer to. I've also considered whether something like "alternative_plan_group" could make sense (since all alternative sub plans will have the same value), but maybe that conveys too much intent on what this is used for. That said, I think for now, to get the buildfarm happy again, v23/0001 seems good. v23/0002 also looks good. Thanks, Lukas -- Lukas Fittl
