Re: Event Triggers reduced, v1

On Fri, Jul 6, 2012 at 4:00 PM, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> And here is another incremental patch, this one doing some more
>> cleanup. Some of this is cosmetic, but it also:
>
> Thanks, applied in my github repository!

Thanks.

>> I have a large remaining maintainability concern about the way we're
>> mapping back and forth between node tags, event tags, and command
>> tags. Right now we've got parse_event_tag, which parses something
> […valid concern…]
>> If you don't have a brilliant idea I'll hack on it and see what I can
>> come up with.
>
> I think we might be able to install a static array for the setup where
> we would find the different elements, and then code up some procedures
> doing different kind of look ups in that array.

+1.

>> like 'ALTER AGGREGATE' into E_AlterAggregate; and then we've got
>> command_to_string, which turns E_AlterAggregate back into 'ALTER
>> AGGREGATE', and then we've got InitEventContext(), which turns
>> T_RenameStmt or T_AlterObjectSchemaStmt with OBJECT_AGGREGATE into
>> E_AlterAggregate. I can't easily verify that all three of these
>
> {
> E_AlterAggregate, // TrigEventCommand
> "ALTER AGGREGATE", // command tag
> T_RenameStmt, // nodeTag
> -1 // object type
> },
> {
> E_AlterAggregate,
> "ALTER AGGREGATE",
> T_AlterObjectSchemaStmt,
> OBJECT_AGGREGATE
> }
>
> The problem is coming up with a way of writing the code that does not
> incur a full array scan for each step of parsing or rewriting. And I
> don't see that it merits yet another cache. Given the existing event
> trigger cache it might be that we don't care about having a full scan of
> this table twice per event trigger related commands, as I don't think it
> would happen when executing other DDLs.
>
> Scratch that we need to parse command tags when we build the event
> cache, so scanning the full array each time would make that O(n²) and we
> want to avoid that. So we could install the contents of the array in
> another hash table in BuildEventTriggerCache() then use that to lookup
> the TrigEventCommand from the command tag…

Ugh. Yeah, obviously the most important thing I think is that
InitEventContext() needs to be lightning-fast, but we don't want
BuildEventTriggerCache() to be pathologically slow either.

I think the best thing to do with InitEventContext() might be to get
rid of it. It's just a big switch over node tags, and we've already
got one of those in standard_ProcessUtility. Maybe every case that
already exists in that function should either (a) get a comment of the
form /* does not support event triggers */ or (b) get a call of the
form EventTriggerStartup(&evt, parsetree, E_WhateverCommandThisIs).
EventTriggerStartup() could call InitEventContext() and then if
CommandFiresTriggersForEvent(..., E_CommandStart) it could also call
ExecEventTriggers(). This might seem like it's just moving the wood
around, but if someone adds a new case in standard_ProcessUtility,
they're going to model it on one of the existing cases, which greatly
decreases the likelihood that they're going to screw it up. And if
they do screw it up it will be obviously non-parallel to the rest of
what's there, so somebody can notice and fix it. As a side benefit,
this would probably be faster than having two separate switches that
are executed more or less consecutively.

Now that leaves the question of how to translate between
E_AlterAggregate and "ALTER AGGREGATE"; I think your idea of a hash
table (or two?) might be the most practical option. We'd only need to
build the hash table(s) if an index-scan of pg_event_trigger finds it
non-empty, and then only once per session.

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