Re: Command Triggers

Hi,

Please find an update attached, v4, fixing most remaining items. Next
steps are better docs and more commands support (along with finishing
currently supported ones), and a review locking behavior.

If you want to just scroll over the patch to get an impression of what's
in there rather than try out the attachment, follow this URL:

https://github.com/dimitri/postgres/compare/master...command_triggers

Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
> Will look into qualifying names.

I'm now qualifying relation names even if they have not been entered
with a namespace qualifier. What do you think? The other components
are left alone, I think the internal APIs for qualifying all kind of
objects from the parse tree and current context are mostly missing.

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

Maybe that's good enough for command triggers?

>> Command triggers should only be allowed for the database owner.
>
> Yes, that needs to happen soon, along with pg_dump and psql support.

All three are implemented in the attached new revision of the patch.

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

Done too. It's better this way, thank you.

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

That's about the way I've done it. Please note that doing it this way
means that a ProcessUtility_hook can decide whether or not the command
triggers are going to be fired or not, and that's true for BEFORE, AFTER
and INSTEAD OF command triggers. I think that's the way to go, though.

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

Fixed. Well, the generic routine would only be called twice and would
only share a rather short expression, so will have to wait until I add
support for more commands.

There's a regression tests gotcha. Namely that the parallel running of
triggers against inheritance makes it impossible to predict if the
trigger on the command CREATE TABLE will spit out a notice in the
inherit tests. I don't know how that is usually avoided, but I guess it
involves picking some other command example that don't conflict with the
parallel tests of that section?

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