On Mon, Aug 7, 2023 at 11:44 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > Right, I doubt that changing that is going to work out well. > Hash joins might have issues with it too.
I thought about the case, because Hash and Hash Join are such closely intertwined nodes, but I don't see any problem there. It doesn't really look like it would matter in what order things got cleaned up. Unless I'm missing something, all of the data structures are just independent things that we have to get rid of sometime. > Could it work to make the patch force child cleanup before parent, > instead of after? Or would that break other places? To me, it seems like the overwhelming majority of the code simply doesn't care. You could pick an order out of a hat and it would be 100% OK. But I haven't gone and looked through it with this specific idea in mind. > On the whole though I think it's probably a good idea to leave > parent nodes in control of the timing, so I kind of side with > your later comment about whether we want to change this at all. My overall feeling here is that what Gather and Gather Merge is doing is pretty weird. I think I kind of knew that at the time this was all getting implemented and reviewed, but I wasn't keen to introduce more infrastructure changes than necessary given that parallel query, as a project, was still pretty new and I didn't want to give other hackers more reasons to be unhappy with what was already a lot of very wide-ranging change to the system. A good number of years having gone by now, and other people having worked on that code some more, I'm not too worried about someone calling for a wholesale revert of parallel query. However, there's a second problem here as well, which is that I'm still not sure what the right thing to do is. We've fiddled around with the shutdown sequence for parallel query a number of times now, and I think there's still stuff that doesn't work quite right, especially around getting all of the instrumentation data back to the leader. I haven't spent enough time on this recently enough to be sure what if any problems remain, though. So on the one hand, I don't really like the fact that we have an ad-hoc recursion arrangement here, instead of using planstate_tree_walker or, as Amit proposes, a List. Giving subordinate nodes control over the ordering when they don't really need it just means we have more code with more possibility for bugs and less certainty about whether the theoretical flexibility is doing anything in practice. But on the other hand, because we know that at least for the Gather/GatherMerge case it seems like it probably matters somewhat, it definitely seems appealing not to change anything as part of this patch set that we don't really have to. I've had it firmly in my mind here that we were going to need to change something somehow -- I mean, the possibility of returning in the middle of node initialization seems like a pretty major change to the way this stuff works, and it seems hard for me to believe that we can just do that and not have to adjust any code anywhere else. Can it really be true that we can do that and yet not end up creating any states anywhere with which the current cleanup code is unprepared to cope? Maybe, but it would seem like rather good luck if that's how it shakes out. Still, at the moment, I'm having a hard time understanding what this particular change buys us. -- Robert Haas EDB: http://www.enterprisedb.com