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

Hi all,

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?

See the attached for the idea.
Thanks,
--
Michael

On Thu, Feb 29, 2024 at 3:21 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> Hi all,
>
> 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?
>
> See the attached for the idea.

why not just shovel these to standard_ProcessUtility.
so ProcessUtility will looking consistent with (in format)
 * ExecutorStart()
 * ExecutorRun()
 * ExecutorFinish()
 * ExecutorEnd()

On Thu, Feb 29, 2024 at 04:10:26PM +0800, jian he wrote:
> why not just shovel these to standard_ProcessUtility.
> so ProcessUtility will looking consistent with (in format)
>  * ExecutorStart()
>  * ExecutorRun()
>  * ExecutorFinish()
>  * ExecutorEnd()

That's one of the points of the change: checking that only in
standard_ProcessUtility() may not be sufficient for utility hooks that
don't call standard_ProcessUtility(), so you'd stil want one in
ProcessUtility().
--
Michael

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

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.
--
Michael

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