On Tue, Feb 8, 2022 at 4:11 PM David Rowley <dgrowle...@gmail.com> wrote: > I still feel this is quite a bit of code for what we're getting here. > I'd be more for it if the path traversal function existed for some > other reason and I was just adding the callback functions and Asserts. > > I'm keen to hear what others think about that.
My view is that functions like path_tree_walker are good things to have on general principle. I find it likely that it will find other uses, and that if we don't add as part of this patch, someone will add it for some other reason in the future. So I would not really count that in deciding how big this patch is, and the rest of what you have here is pretty short and to the point. There is the more difficult philosophical question of whether it's worth expending any code on this at all. I think it is pretty clear that this has positive value: it could easily prevent >0 future bugs, which IMHO is not bad for such a small patch. However, it does feel a little bit primitive somehow, in the sense that there are a lot of things you could do wrong which this wouldn't catch. For example, a Gather with no parallel-aware node under it is probably busted, unless someone invents new kinds of parallel operators that work differently from what we have now. But a join beneath a Gather that is not itself parallel-aware should have a parallel-aware node under exactly one side of the join. If there's a parallel scan on both sides or neither side, even with stuff on top of it, that's wrong. But a parallel-aware join could do something else, e.g. Parallel Hash Join expects a parallel path on both sides. Some other parallel-aware join type could expect a parallel path on exactly one side without caring which one, or on one specific side, or maybe even on neither side. What we're really reasoning about here is whether the input is going to be partitioned across multiple executions of the plan in a proper way. A Gather is going to run the same plan in all of its workers, so it wants a subplan that when run in all workers will together produce all output rows. Parallel-aware scans partition the results across workers, so they behave that way. A non-parallel aware join will work that way if it joins a partition the input on one side to all of the input from the other side, hence the rule I describe above. For aggregates, you can't safely apply a plain old Aggregate operation either to a regular scan or to a parallel-aware scan and get the right answer, which is why we need Partial and Finalize stages for parallel query. But for a lot of other nodes, like Materialize, their output will have the same properties as the input: if the subplan of a Materialize node produces all the rows on each execution, the Materialize node will too; if it produces a partition of the output rows each time it's executed, once per worker, the Materialize node will do the same. And I think it's that kind of case that leads to the check we have here, that there ought to be a parallel-aware node in there someplace. It might be the case that there's some more sophisticated check we could be doing here that would be more satisfying than the one you've written, but I'm not sure. Such a check might end up needing to know the behavior of the existing nodes in a lot of detail, which then wouldn't help with finding bugs in new functionality we add in the future. In that sense, the kind of simple check you've got here has something to recommend it: it won't catch everything people can do wrong, but when it does trip, chances are good it's found a bug, and it's got a good chance of continuing to work as well as it does today even in the face of future additions. So I guess I'm mildly in favor of it, but I would also find it entirely reasonable if you were to decide it's not quite worth it. -- Robert Haas EDB: http://www.enterprisedb.com
