Re: Command Triggers

On Thu, Feb 16, 2012 at 4:21 PM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> We could decide command triggers are superuser only.  Security is not
> something I'm very strong at, so I'll leave it up to you to decide.

That's certainly the easiest option. If you don't feel passionate
about spending a lot of energy figuring out how to make it secure,
then I suggest we just restrict it to superusers until someone does.

>> If I install a command trigger that prevents all DDL, how do I
>> uninstall it?  Or say I'm the superuser and I want to undo something
>> my disgruntled DBA did before he quit.
>
> Good catch, I guess we need to remove creating and dropping a command
> trigger to the list of supported commands in the ANY COMMAND list.

Another option would be to add a PGC_SUSET boolean GUC that can be
used to disable command triggers. I think that might be more
flexible, not to mention useful for recursion prevention.

>> I am a bit worried about supporting command triggers on statements
>> that do internal transaction control, such as VACUUM and CREATE INDEX
>> CONCURRENTLY.  Obviously that's very useful and I'd like to have it,
>> but there's a problem: if the AFTER trigger errors out, it won't undo
>> the whole command.  That might be very surprising.  BEFORE triggers
>> seem OK, and AFTER triggers might be OK too but we at least need to
>> think hard about how to document that.
>
> I think we should limit the support to BEFORE command trigger only.  It
> was unclear to me how to solve the problem for the AFTER command case,
> and if you're unclear to, then there's not that many open questions.

Works for me.

>> I think it would be better to bail on trying to use CREATE TRIGGER and
>> DROP TRIGGER as a basis for this functionality, and instead create
>> completely new toplevel statements CREATE COMMAND TRIGGER and DROP
>> COMMAND TRIGGER.  Then, you could decide that all command triggers
>> live in the same namespace, and therefore to get rid of the command
>> trigger called bob you can just say "DROP COMMAND TRIGGER bob",
>> without having to specify the type of command it applies to.  It's
>
> I have no strong feeling about that.  Would that require that COMMAND be
> more reserved than it currently is (UNRESERVED_KEYWORD)?

It shouldn't.

>> still clear that you're dropping a *command* trigger because that's
>> right in the statement name, whereas it would make me a bit uneasy to
>> decide that "DROP TRIGGER bob" means a command trigger just by virtue
>> of the fact that no table name was specified.  That would probably
>> also make it easier to accomplish the above-described goal of
>> integrating this into the DropStmt infrastructure.
>
> The other thing is that you might want to drop the trigger from only one
> command, of course we could support both syntax.  We could also add
> support for the following one:
>
>  DROP TRIGGER bob ON ALL COMMANDS;

Uh, hold on. Creating a trigger on multiple commands ought to only
create one entry in pg_cmdtrigger. If you drop it, you drop the whole
thing. Changing which commands the trigger applies to would be the
job for ALTER, not DROP. But note that we have no similar
functionality for regular triggers, so I can't think we really need it
here either.

>> Can we call InitCommandContext in some centralized place, rather than
>> separately at lots of different call sites?
>
> Then you have to either make the current command context a backend
> private global variable, or amend even more call sites to pass it down.
> The global variable idea does not work, see RemoveRelations() and
> RemoveObjects() which are handling an array of command contexts.
>
> So do you prefer lots of InitCommandContext() or adding another parameter
> to almost all functions called by standard_ProcessUtility()?

Blech. Maybe we should just have CommandFiresTriggers initialize it
if not already done?

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