On Wed, Dec 3, 2025 at 11:03 PM Robert Haas <[email protected]> wrote: > It's a reasonable concern, but I think we should go ahead with the > patch anyway and see what happens. I believe that comment emerged from > some problems with buildfarm instability around the time that code was > committed -- I want to say that the buildfarm turned red, Tom firmly > informed me that some of the choices I had made were not for the best, > and I revised things accordingly including adding that comment, but I > might be remembering the history incorrectly. Regardless, if it turns > out there's a problem, we'll have to do something more complicated, > which I assume would mean either adjusting the test cases, or if that > seems like the wrong approach, then either making a separate > RelOptInfo, or re-adding some of the paths from the previous path > list. But I'm reluctant to do any of that without seeing something > actually go wrong, because who is to say that whatever I change will > fix whatever the problem is?
Fair point. I had envisioned a separate planning step involving a new RelOptInfo, where we would re-add paths from the scan/join RelOptInfo after applying the target, explicitly skipping Append paths for partitioned tables. But I admit that I am unsure if this addresses any real problems, so the effort might not be justified. I agree that maybe we should just go ahead with your current patch and see what happens. > Long term, i wonder whether we can find some overall better way to > handle the toplevel scan/join target. Why do we even go to the trouble > of generating paths with the wrong scan/join target first and then fix > them all, instead of getting the scan/join target right from the > beginning? I suspect the original answer is that there was no real > downside because it couldn't change which path appeared cheapest and > surgically inserting the final target seemed easier than making sure > to have the correct target available when the paths were originally > generated. But that was before partitionwise join, before declarative > partitioning, before parallel query, and... I would guess probably not > before table inheritance, but I don't know for sure. As we've added > more features to the system, maintaining this after-the-fact scan/join > target insertion has become more and more of a hack, and I bet the > right solution is to figure out a way to make the target list > available earlier. I suspect the same. IMHO, apply_scanjoin_target_to_paths is quite brute-force and modifies planner structures in ways we generally should avoid (no offense intended to the original author of this function). I agree that the correct long-term solution is to generate the expected target from the start, which would eliminate the need for apply_scanjoin_target_to_paths entirely. > That seems like it would avoid this class of > problems a lot more thoroughly, but it also seems like a bigger > project that doesn't really make sense in the context of fixing the > present issue. Yes, that is definitely beyond the scope of the fix we are discussing here. - Richard
