On Fri, May 17, 2024 at 10:11 PM Michael Paquier <mich...@paquier.xyz> wrote: > On Fri, May 17, 2024 at 03:53:58PM -0400, Robert Haas wrote: > > 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. > > Perhaps, still that would be something. > > Hmm. We've had in the past bugs where DDL paths were playing with the > manipulation of Querys in core, corrupting their contents. It seems > like what you would mean is to have a check at the *end* of > ProcessUtility() that does an equalFuncs() on the Query, comparing it > with a copy of it taken at its beginning, all that hidden within two > USE_ASSERT_CHECKING blocks, when we'd expect the tree to not change. > With readOnlyTree, that would be just changing from one policy to > another, which is not really interesting at this stage based on how > ProcessUtility() is shaped.
I don't think I meant to imply that. I think what I feel is that there's no real problem here, and that the patch sort of superficially looks useful, but isn't really. I'd suggest just dropping it. -- Robert Haas EDB: http://www.enterprisedb.com