On Fri, May 17, 2024 at 10:11 PM Michael Paquier <michael@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
