Обсуждение: Re: [COMMITTERS] pgsql: Add sql_drop event for event triggers

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> Add sql_drop event for event triggers

The buildfarm members that use -DCLOBBER_CACHE_ALWAYS don't like this
patch:

***************
*** 760,771 ****
      FROM generate_series(1, 50) a;
  BEGIN;
  SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
  UPDATE serializable_update_tab SET description = 'no no', id = 1 WHERE id = 1;
  COMMIT;
  SELECT description FROM serializable_update_tab WHERE id = 1;
!     description
! --------------------
!  updated in trigger
  (1 row)

  DROP TABLE serializable_update_tab;
--- 760,773 ----
      FROM generate_series(1, 50) a;
  BEGIN;
  SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
+ ERROR:  SET TRANSACTION ISOLATION LEVEL must be called before any query
  UPDATE serializable_update_tab SET description = 'no no', id = 1 WHERE id = 1;
+ ERROR:  current transaction is aborted, commands ignored until end of transaction block
  COMMIT;
  SELECT description FROM serializable_update_tab WHERE id = 1;
!  description
! -------------
!  new
  (1 row)

  DROP TABLE serializable_update_tab;

I suspect you have inserted a snapshot-capturing operation into
someplace it mustn't go during transaction startup, but only in a path
that is triggered by an immediately preceding cache flush.

            regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre@alvh.no-ip.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

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> (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.)

I think it would be difficult and probably dangerous to have PG_TRY
for only some utility commands, so not much to be done about that.
The main thing is to not invoke event trigger code for BEGIN/ABORT/SET.

            regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>> (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.)
>
> I think it would be difficult and probably dangerous to have PG_TRY
> for only some utility commands, so not much to be done about that.
> The main thing is to not invoke event trigger code for BEGIN/ABORT/SET.

What about splitting the big switch statement into two of them? The
first one for transaction control statements, and then the other bigger
one.

Maybe we could even rework the code (either in some other switch
statements or just by physical lines proximity) so that TCL, DCL, DDL,
etc are each in easy to spot blocks, which is more or less true as of
today, but not exactly so IIRC.

Then we don't need new support code, and we can even continue using the
current macro.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

Dimitri Fontaine <dimitri@2ndquadrant.fr> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> I think it would be difficult and probably dangerous to have PG_TRY
>> for only some utility commands, so not much to be done about that.
>> The main thing is to not invoke event trigger code for BEGIN/ABORT/SET.

> What about splitting the big switch statement into two of them? The
> first one for transaction control statements, and then the other bigger
> one.

Sounds like considerable uglification to fix a performance issue that's
entirely hypothetical... let's see some numbers that prove it's worth
worrying about before we do that.

            regards, tom lane

I wrote:
> Dimitri Fontaine <dimitri@2ndquadrant.fr> writes:
>> What about splitting the big switch statement into two of them? The
>> first one for transaction control statements, and then the other bigger
>> one.

> Sounds like considerable uglification to fix a performance issue that's
> entirely hypothetical... let's see some numbers that prove it's worth
> worrying about before we do that.

Actually ... wait a moment.  That does have some attraction independent
of performance questions, because what Alvaro suggested requires knowing
which commands support command triggers in two places.  Perhaps with
some refactoring we could end up with no net addition of cruft.

Personally, I'd really like to see the InvokeDDLCommandEventTriggers
macros go away; that's not a coding style I find nice.  If we had a
separate switch containing just the event-supporting calls, we could
drop that in favor of one invocation of the trigger stuff before and
after the switch.

            regards, tom lane

On Tue, Apr 9, 2013 at 12:07 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
>> Dimitri Fontaine <dimitri@2ndquadrant.fr> writes:
>>> What about splitting the big switch statement into two of them? The
>>> first one for transaction control statements, and then the other bigger
>>> one.
>
>> Sounds like considerable uglification to fix a performance issue that's
>> entirely hypothetical... let's see some numbers that prove it's worth
>> worrying about before we do that.
>
> Actually ... wait a moment.  That does have some attraction independent
> of performance questions, because what Alvaro suggested requires knowing
> which commands support command triggers in two places.  Perhaps with
> some refactoring we could end up with no net addition of cruft.
>
> Personally, I'd really like to see the InvokeDDLCommandEventTriggers
> macros go away; that's not a coding style I find nice.  If we had a
> separate switch containing just the event-supporting calls, we could
> drop that in favor of one invocation of the trigger stuff before and
> after the switch.

I kind of wonder if there's some way we could split ProcessUtility()
up into more digestible pieces.  I can't really think of a good way to
do it though, without writing duplicative switches.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Tom Lane <tgl@sss.pgh.pa.us> writes:
> Actually ... wait a moment.  That does have some attraction independent
> of performance questions, because what Alvaro suggested requires knowing
> which commands support command triggers in two places.  Perhaps with
> some refactoring we could end up with no net addition of cruft.

That was the idea yes, that's the right context now.

About the refactoring itself, how much do we want to keep the compiler
help about covering all the items in the switch? Other than that,
changing the breaks into returns in the first switch looks like it would
do the work, we don't even need a goto cleanup.

> Personally, I'd really like to see the InvokeDDLCommandEventTriggers
> macros go away; that's not a coding style I find nice.  If we had a
> separate switch containing just the event-supporting calls, we could
> drop that in favor of one invocation of the trigger stuff before and
> after the switch.

That needs either lots of code duplication or some smarts that I don't
see yet, because of the EventTriggerSupportsObjectType stuff. Anyways
I'm not much into C macrology myself…

At best I can find some time and work on that from Thursday.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

Robert Haas <robertmhaas@gmail.com> writes:
> I kind of wonder if there's some way we could split ProcessUtility()
> up into more digestible pieces.  I can't really think of a good way to
> do it though, without writing duplicative switches.

I'm thinking a bit about

    ProcessUtility()
    {
        switch (tag)
        {
            ... cases for BEGIN etc ...
            default:
                ProcessSlowUtility(...)
        }
    }

    ProcessSlowUtility()
    {
        event setup code
        switch (tag)
        {
            ... cases for everything else ...
            default:
                elog(ERROR)
        }
        event teardown code
    }

            regards, tom lane

Dimitri Fontaine <dimitri@2ndquadrant.fr> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> Personally, I'd really like to see the InvokeDDLCommandEventTriggers
>> macros go away; that's not a coding style I find nice.  If we had a
>> separate switch containing just the event-supporting calls, we could
>> drop that in favor of one invocation of the trigger stuff before and
>> after the switch.

> That needs either lots of code duplication or some smarts that I don't
> see yet, because of the EventTriggerSupportsObjectType stuff. Anyways
> I'm not much into C macrology myself…

Yeah, I was just looking at the IfSupported variant.  In the structure
I just suggested (separate ProcessSlowUtility function), we could make
that work by having switch cases for some statements in both functions,
perhaps like this:

    RenameStmt:
        if (stmt allows event triggers)
            ProcessSlowUtility(...);
        else
            ExecRenameStmt(stmt);
        break;

while in ProcessSlowUtility it'd just look normal:

    RenameStmt:
        ExecRenameStmt(stmt);
        break;

This would also get rid of the assumption that's currently wired into
InvokeDDLCommandEventTriggersIfSupported that the only sort of dynamic
test that can be needed is an EventTriggerSupportsObjectType call.
In the sketch above, the if() could be testing any property of the stmt.

            regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:
> Yeah, I was just looking at the IfSupported variant.  In the structure
> I just suggested (separate ProcessSlowUtility function), we could make
> that work by having switch cases for some statements in both functions,
> perhaps like this:

Ah yes, that's minimal code duplication and cleaner effect.

>     RenameStmt:
>         if (stmt allows event triggers)
>             ProcessSlowUtility(...);
>         else
>             ExecRenameStmt(stmt);
>         break;
>
> while in ProcessSlowUtility it'd just look normal:
>
>     RenameStmt:
>         ExecRenameStmt(stmt);
>         break;

I like it globally. Do you think some inline magic needs to happen to
try and convince the compiler to process the whole thing as a single
function? My understanding is that while there's no way to require the
inlining to happen we still have some provisions to hint the compilers
wanting to listen, or something like that.

> This would also get rid of the assumption that's currently wired into
> InvokeDDLCommandEventTriggersIfSupported that the only sort of dynamic
> test that can be needed is an EventTriggerSupportsObjectType call.
> In the sketch above, the if() could be testing any property of the stmt.

And even better, could easily be made different from a call site to the
next, by simply upgrading the complex command into the main utility
switch.

Do you want me to work on a patch at the end of this week?

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

Dimitri Fontaine wrote:
> Tom Lane <tgl@sss.pgh.pa.us> writes:

> >     RenameStmt:
> >         if (stmt allows event triggers)
> >             ProcessSlowUtility(...);
> >         else
> >             ExecRenameStmt(stmt);
> >         break;
> >
> > while in ProcessSlowUtility it'd just look normal:
> >
> >     RenameStmt:
> >         ExecRenameStmt(stmt);
> >         break;
>
> I like it globally. Do you think some inline magic needs to happen to
> try and convince the compiler to process the whole thing as a single
> function? My understanding is that while there's no way to require the
> inlining to happen we still have some provisions to hint the compilers
> wanting to listen, or something like that.

I don't see how inlining could work here.  We will end up with a couple
dozen calls to ProcessSlowUtility inside ProcessUtility, so inlining it
will be a really poor strategy.

> Do you want me to work on a patch at the end of this week?

As (one of) the committer(s) responsible for this code, I do, thanks.

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

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> I don't see how inlining could work here.  We will end up with a couple
> dozen calls to ProcessSlowUtility inside ProcessUtility, so inlining it
> will be a really poor strategy.

Maybe I should have spent some time thinking about the idea rather than
just writing it down. Thanks.

>> Do you want me to work on a patch at the end of this week?
>
> As (one of) the committer(s) responsible for this code, I do, thanks.

Will do.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

Tom Lane <tgl@sss.pgh.pa.us> writes:
> Yeah, I was just looking at the IfSupported variant.  In the structure
> I just suggested (separate ProcessSlowUtility function), we could make
> that work by having switch cases for some statements in both functions,

I've done it the way you propose here, and then in the Slow variant we
have two set of cases again: those with some manual transactionnal
behavior or some other code complexities, and the really simple ones.

The attached patch involves a second layer of distinction to simplify
the code fully and remove all the Event Trigger related macrology that I
didn't much like. Maybe that's going a tad too far, see what you think.

Of course the patch passes make check.

Also, some switch cases are now duplicated for the sole benefit of
keeping some compiler help, but I don't know how much help we're talking
about now that we have 3 different switches. It seemed cleaner to do it
that way as it allows to ERROR out in the very last default case.

Finally, I've been surprised to find out that those cases are only
triggering for "ddl_event_start" (and not "ddl_event_end"), I think
that's a bug we should be fixing:

        case T_AlterDomainStmt:
        case T_DefineStmt:
        case T_IndexStmt:        /* CREATE INDEX */

The T_IndexStmt should be triggering ddl_event_end when stmt->concurrent
is false, and that's not the case in the code in the master's branch.
The two other cases should just be moved to the later switch so that
both start and end triggers happen.

Or maybe I'm missing something here. Given that it might as well be the
case, I did only refactor the code keeping its current behavior rather
than fixing what I think is a bug while doing so.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> Yeah, I was just looking at the IfSupported variant.  In the structure
>> I just suggested (separate ProcessSlowUtility function), we could make
>> that work by having switch cases for some statements in both functions,

> I've done it the way you propose here, and then in the Slow variant we
> have two set of cases again: those with some manual transactionnal
> behavior or some other code complexities, and the really simple ones.

I started to look at this patch.  What in the world is the point of
dividing the "slow" function into two separate switches?  Seems like
you might as well put all the cases in the first switch back into
standard_ProcessUtility.

            regards, tom lane

Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> Yeah, I was just looking at the IfSupported variant.  In the structure
>> I just suggested (separate ProcessSlowUtility function), we could make
>> that work by having switch cases for some statements in both functions,

> I've done it the way you propose here, and then in the Slow variant we
> have two set of cases again: those with some manual transactionnal
> behavior or some other code complexities, and the really simple ones.

> The attached patch involves a second layer of distinction to simplify
> the code fully and remove all the Event Trigger related macrology that I
> didn't much like. Maybe that's going a tad too far, see what you think.

Applied with some further hacking.

> Of course the patch passes make check.

Hmm, that leads me to wonder exactly how extensively the regression
tests test event triggers, because it sure looked to me like there
were multiple bugs left in this version.

> Finally, I've been surprised to find out that those cases are only
> triggering for "ddl_event_start" (and not "ddl_event_end"), I think
> that's a bug we should be fixing:

Agreed, I fixed it.

            regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:
> Applied with some further hacking.

Thanks!

> Hmm, that leads me to wonder exactly how extensively the regression
> tests test event triggers, because it sure looked to me like there
> were multiple bugs left in this version.

The first versions of the event triggers patch series used to include a
large regression test files with some level of covering for each and
every supported command. It was used this way because there was some
command specific code at the time.

It was apparently not testing the right things and was judged to be too
much tests for no clear benefit, thus thrown away. Then with the never
stopping refactoring of the patch series, no specific regression tests
ever found their way back in.

It's not clear to me what tests to add exactly, will find some time to
read the current code in more details and figure out what's easy enough
to cover and not covered already.

>> Finally, I've been surprised to find out that those cases are only
>> triggering for "ddl_event_start" (and not "ddl_event_end"), I think
>> that's a bug we should be fixing:
>
> Agreed, I fixed it.

Thanks!

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support