Re: Command Triggers

Hi,

Please find attached version 9 of this patch, answering to most open
comments about it.

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> 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.

Done.

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

Done.

> 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 implemented support for the session_replication_role GUC so that you
can SET session_replication_role TO replica; and avoid any command
trigger running (if you didn't ALTER them from their default setting).

Of course you can also use the GUC in its intended purpose, as said
Marko.

>>> [CREATE INDEX CONCURRENTLY and VACUUM]
>> 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.

Done. Of course at the time the command trigger is created you can't
distinguish if the CREATE INDEX command will be run CONCURRENTLY or not,
so I've decided to issue a WARNING about it.

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

Not done yet, I'm not sure we took a decision on this one. Also note
that I chose to give DDL on command triggers their own command tag so
that it's easy to separate them out in the command trigger context.

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> Wait, we already have ALTER TRIGGER bob ON ANY COMMAND SET DISABLED;
> Eh, so what happens then if someone sets a command trigger on ALTER TRIGGER?

You would need to set a command trigger on ALTER COMMAND TRIGGER and
that's not supported. Triggers on command "ALTER TRIGGER" in fact will
not get fired on ALTER TRIGGER ... ON COMMAND ...

I guess that's a point to change the grammar the way you're hinting:

CREATE COMMAND TRIGGER
DROP COMMAND TRIGGER
ALTER COMMAND TRIGGER

That also needs each their own reference page. It will be easier on the
users I guess. Will work on that.

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I'm OK with not supporting command triggers on command triggers, but I
> still think the GUC is useful. Keep in mind that flipping a GUC is
> really cheap compared to a catalog change, and can affect just one
> session. Those are significant advantages. However, if you want to
> just not support triggers on statements that modify command triggers,
> I'm OK with that, too.

Both done, if you agree with using session_replication_role here.

> Sure, but if you run the same trigger on multiple operations - INSERT
> OR UPDATE OR DELETE.

I failed to see that analogy. The other problem with the current way of
doing things is that I can't integrate with RemoveObjects(), and I think
you won't like that :)

>> You create a trigger on ANY command :)
>
> Oh. Well, then +1 for me on the ANY COMMAND thing, but -1 on ALL
> COMMANDS. I can't see that there's enough utility to having a
> bulk-create functionality to justify its existence. The ANY COMMAND
> thing I think is what people will want.

There's no such thing as ALL COMMANDS in the patch, there's a syntactic
sugar allowing you to create and drop more than one command trigger in a
single command, much as we have DROP TABLE foo, bar, baz;

> I am thinking that we should ditch the idea of keeping track of
> commands using strings and instead assign a bunch of integer constants
> using a big enum. The parser can translate from what the user enters
> to these constants and then use those throughout, including in the
> system catalogs.

It's not really command strings but the Command Tag we've historically
been using up until now. You're saying that it should remain the same
for users but change internally. No strong opinion from me here, apart
from it being more code for doing the same thing.

I mean we will need to get the command tag from the parse tree then
change that into an integer thanks to yet another huge switch that will
have to be maintained every time we add a new command. We only have too
many of them now (not proposing to remove some).

> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
>> I didn't like the new cmdtrigger.h file. It's included by a lot of
>> other headers, and it's also itself including execnodes.h and

Fixed in the attach.

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