Re: Propagate sanity checks of ProcessUtility() to standard_ProcessUtility()?

On Thu, Feb 29, 2024 at 2:21 AM Michael Paquier <michael(at)paquier(dot)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