Re: Command Triggers

On Saturday, December 03, 2011 12:04:57 AM Dimitri Fontaine wrote:
> Hi,
>
> First thing first: thank you Andres for a great review, I do appreciate
> it. Please find attached a newer version of the patch. The github
> repository is also updated.
>
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > I think at least a
> > * command_tag text
>
> Added.
>
> > Why is there explicit documentation of not checking the arguments of said
> > trigger function? That seems to be a bit strange to me.
> The code is searching for a function with the given name and 5 text
> arguments, whatever you say in the command. That could be changed later
> on if there's a need.
Forget it, i was too dumb to remember the code when I wrote the above ;)

> > schemaname currently is mightily unusable because whether its sent or not
> > depends wether the table was created with a schemaname specified or not.
> > That doesn't seem to be a good approach.
> I'm not sure about that, we're spiting out what the user entered.
No, were not. Were writing out something that semantically the same what the
user entered. At several places NULL/NOT NULL and such get added.

> > As an alternative it would be possible to pass the current search path
> > but that doesn't seem to the best approach to me;
> The trigger runs with the same search_path settings as the command, right?
For many things it makes sense to specify the search path of a function uppon
creation of the function. Even moreso if you have security definer
functions...
Otherwise you can get into troubles via operators and such...

> > Then there is nice stuff like CREATE TABLE .... (LIKE ...);
> > What shall that return in future? I vote for showing it as the plain
> > CREATE TABLE where all columns are specified.
> I don't think so. Think about the main use cases for this patch,
> auditing and replication. Both cases you want to deal with what the
> user said, not what the system understood.
Is that so? In the replication case the origin table very well may look
differently on the master compared to the standby. Which is about impossible
to diagnose with the above behaviour.

> > I think it would sensible to allow multiple actions on which to trigger
> > to be specified just as you can for normal triggers. I also would like
> > an ALL option
> Both are now implemented.
Cool, will check.

> When dealing with an ANY command trigger, your procedure can get fired
> on a command for which we don't have internal support for back parsing
> etc, so you only get the command tag not null. I think that's the way to
> go here, as I don't want to think we need to implement full support for
> command triggers on ALTER OPERATOR FAMILY from the get go.
Thats ok with me.

> > Currently the grammar allows options to be passed to command triggers. Do
> > we want to keep that? If yes, with the same arcane set of datatypes as
> > in data triggers?
> > If yes it needs to be wired up.
> In fact it's not the case, you always get called with the same 5
> arguments, all nullable now that we have ANY COMMAND support.
I think you misunderstood me. Check the following excerpt from gram.y:
CreateCmdTrigStmt:
CREATE TRIGGER name TriggerActionTime COMMAND trigger_command_list
EXECUTE PROCEDURE func_name '(' TriggerFuncArgs ')'

You have TriggerfuncArgs there but you ignore them.

> > * schema qualification:
> > An option to add triggers only to a specific schema would be rather neat
> > for many scenarios.
> > I am not totally sure if its worth the hassle of defining what happens in
> > the edge cases of e.g. moving from one into another schema though.
> I had written down support for a WHEN clause in command triggers, but
> the exact design has yet to be worked out. I'd like to have this
> facility but I'd like it even more if that could happen in a later
> patch.
Hm. I am not sure a generic WHEN clause is needed. In my opinion schema
qualification would be enough...
A later patch is fine imo.

> > I contrast to data triggers command triggers cannot change what is done.
> > They can either prevent it from running or not. Which imo is fine.
> > The question I have is whether it would be a good idea to make it easy to
> > prevent recursion.... Especially if the triggers wont be per schema.
>
> You already have
>
> ATER TRIGGER foo ON COMMAND create table DISABLE;
>
> that you can use from the trigger procedure itself. Of course, locking
> is an issue if you want to go parallel on running commands with
> recursive triggers attached. I think we might be able to skip solving
> that problem in the first run.
hm. Not yet convinced. I need to think about it a bit.

Have fun ;)

Andres