On Thu, Feb 29, 2024 at 2:21 AM Michael Paquier <mich...@paquier.xyz> wrote: > It's been brought to me that an extension may finish by breaking the > assumptions ProcessUtility() relies on when calling > standard_ProcessUtility(), causing breakages when passing down data to > cascading utility hooks. > > Isn't the state of the arguments given something we should check not > only in the main entry point ProcessUtility() but also in > standard_ProcessUtility(), to prevent issues if an extension > incorrectly manipulates the arguments it needs to pass down to other > modules that use the utility hook, like using a NULL query string?
I can't imagine a scenario where this change saves more than 5 minutes of debugging, so I'd rather leave things the way they are. If you do this, then people will see the macro and have to go look at what it does, whereas right now, they can see the assertions themselves, which is better. The usual pattern for using hooks like this is to call the next implementation, or the standard implementation, and pass down the arguments that you got. If you try to do that and mess it up, you're going to get a crash really, really quickly and it shouldn't be very difficult at all to figure out what you did wrong. Honestly, that doesn't seem like it would be hard even without the assertions: for the most part, a quick glance at the stack backtrace ought to be enough. If you're trying to do something more sophisticated, like mutating the node tree before passing it down, the chances of your mistakes getting caught by these assertions are pretty darn low, since they're very superficial checks. -- Robert Haas EDB: http://www.enterprisedb.com
