From: | Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> |
---|---|
To: | Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> |
Cc: | Andres Freund <andres(at)anarazel(dot)de>, pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: Command Triggers |
Date: | 2011-12-05 21:15:20 |
Message-ID: | m2ipluhg9j.fsf@2ndQuadrant.fr |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
command-trigger.v4.patch.gz | application/octet-stream | 26.9 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Bruce Momjian | 2011-12-05 21:46:58 | Re: pg_upgrade and regclass |
Previous Message | Robert Haas | 2011-12-05 20:16:20 | Re: hiding variable-length fields from Form_pg_* structs |