Re: [COMMITTERS] pgsql: Add sql_drop event for event triggers

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> writes:
> > Add sql_drop event for event triggers
>
> The buildfarm members that use -DCLOBBER_CACHE_ALWAYS don't like this
> patch:

Ah. Andres Freund found it, after Dimitri prodded me on IM after
Andrew's today mailing list post -- the problem is the new
UTILITY_BEGIN_QUERY macro. Or, more accurately, the fact that this
macro is called for every ProcessUtility invocation, regardless of the
command being a supported one or not.

The current coding is bogus not only because it causes the problem we're
seeing now, but also because it's called during aborted transactions,
for example, which is undesirable. It also causes extra overhead during
stuff as simple as BEGIN TRANSACTION. So we do need to fix this
somehow.

So obviously we need to avoid calling that unless necessary. I think
there are two ways we could go about this:

1. Take out the call at the top of the function, and only call it in
specific cases within the large switch.

2. Make the single call at the top conditional on node type and
arguments therein.

I think I like (2) best; we could write a separate function returning
boolean which receives parsetree, which would return true when the
command supports event triggers. Then we can redefine
UTILITY_BEGIN_QUERY() to look like this:

#define UTILITY_BEGIN_QUERY(isComplete, parsetree) \
do { \
bool _needCleanup = false; \
\
if (isComplete && EventTriggerSupportsCommand(parsetree)) \
{ \
_needCleanup = EventTriggerBeginCompleteQuery(); \
} \
\
PG_TRY(); \
{ \
/* avoid empty statement when followed by a semicolon */ \
(void) 0

and this solves the problem nicely AFAICT.

(Someone might still complain that we cause a PG_TRY in BEGIN
TRANSACTION etc. Not sure if this is something we should worry about.
Robert did complain about this previously.)

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services