Re: Command Triggers - Mailing list pgsql-hackers

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

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

> Imo the output should fully schema qualify anything including operators. Yes,
> thats rather ugly but anything else seems to be too likely to lead to
> problems.

Will look into qualifying names.

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

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

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

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.

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

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

> Currently there is no locking strategy at all. Which e.g. means that there is
> no protection against two sessions changing stuff concurrently or such.
>
> Was that just left out - which is ok - or did you miss that?

I left out the locking as of now.  I tried to fix some of it in the new
patch though, but I will need to spend more time on that down the road.

> Command triggers should only be allowed for the database owner.

Yes, that needs to happen soon, along with pg_dump and psql support.

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

> Imo the current callsite in ProcessUtility isn't that good. I think it would
> make more sense moving it into standard_ProcessUtility. It should be *after*
> the check_xact_readonly check in there in my opinion.

Makes sense, will fix in the next revision.

> I am also don't think its a good idea to run the
> ExecBeforeOrInsteadOfCommandTriggers stuff there because thats allso the path
> that COMMIT/BEGIN and other stuff take. Those are pretty "fast path".
>
> I suggest making two switches in standard_ProcessUtility, one for the non-
> command trigger stuff which returns on success and one after that. Only after
> the first triggers are checked.

Agreed.

> Also youre very repeatedly transforming the parstree into a string. It would
> be better doing that only once instead of every trigger...

It was only done once per before and instead of triggers (you can't have
both on the same command), and another time for any and all after
triggers, meaning at most twice. It's now down to only once, at most.

> * many functions should be static but are not. Especially in cmdtrigger.c

Fixed.

> * Either tgenable's datatype or its readfunc is wrong (watch the compiler ;))
> * ruleutils.c:
>   * generic routine for IF EXISTS, CASCADE, ...

Will have to wait until next revision.


Thanks you again for a great review, regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support