On Thu, Oct 3, 2024 at 5:52 PM David Rowley <dgrowle...@gmail.com> wrote: > It looks fine with the patch. The crux of the new logic is just > summing up the disabled_nodes from the child nodes and checking if the > disabled_nodes of the current node is higher than that sum. That's not > exactly hard logic. The biggest risk seems to be not correctly > visiting all the child nodes. I got that wrong with SubqueryScan in my > first PoC.
Right, visiting too many or too few child nodes would be bad. The other worry I have is about things that aren't fully Pathified -- maybe a certain node doesn't have a representation in the Path tree but gets injected at Plan time. In that case there might be a risk of the Disabled marker showing up in the wrong place. We do this with sorts, for example, in the merge-join case. > which isn't correct. Append appears disabled, but it's not. Sort is. > Before I fixed that in the patch, I was incorrectly getting the > "Disabled: true" under the Append. I feel we're more likely to get bug > reports alerting us to incorrect logic when the disabled property only > appears on disabled nodes as there are far fewer of them to look at > and therefore it's more obvious when they're misplaced. It's certainly possible that you're correct, and the fact that you have this example in hand makes it more likely. I tend to gravitate toward displaying things exactly as they exist internally because I've had so many bad experiences with having to try to reverse-engineer the value stored internally from whatever is printed. This problem isn't limited to EXPLAIN, but to give one EXPLAIN-related example, we take the row count and divide by nloops and round off to an integer and I cannot tell you how many times that has made my life more difficult because I really want to see planstate->instrument->ntuples, not round(planstate->instrument->ntuples/nloops); likewise, I absolutely loathe the fact that we round off plan->plan_rows to an integer. I guess we do that so that we "don't confuse people," but what it means is that I can't see the information I need to help people fix problems, and frankly what it means is that I myself am confused. I tend to feel like if the problem is that a user does not understand what we are printing, that problem can be fixed by the user learning more until they understand; but if the problem is that we don't print enough information to understand what is truly happening inside the data structure, there is no way out from under that problem without recompiling, which is not where you want to be when something goes wrong in production. Of course that idea can be taken too far. If you refuse to translate information into a more human-understandable form even when you can do so reliably, then you're just making life hard for users for no benefit, and you might be right that this is such a case. I'm not here to act like I have all the right answers. I'm just explaining the reasoning behind what I did; and I hope that it makes some sense to you. -- Robert Haas EDB: http://www.enterprisedb.com
