Thread: Adding the optional clause 'AS' in CREATE TRIGGER
Hi, I would like to add the following support for a trigger. This implementation enables to create a trigger efficiently in single command. It had been discussed before. The link is as shown below. https://www.postgresql.org/message-id/CAA-aLv4m%3Df9cc1zcUzM49pE8%2B2NpytUDraTgfBmkTOkMN_wO2w%40mail.gmail.com Currently, PostgreSQL requires two steps to create a trigger. 1. to create a function. 2. to define a trigger with action specified via already created function. Supporting 'AS' clause in CREATE TRIGGER syntax will enable the option of defining the trigger in single command. As a bonus, it will be compatible with oracle. Also, the optional clause 'OR REPLACE' is required as below. https://www.postgresql.org/message-id/CAA-aLv6KYgVt2CwaRdcnptzWVngEm72Cp4mUFnF-MfeH0gS91g%40mail.gmail.com Currently, to change the definition of a trigger, trigger needs to be dropped first before creating it again with new definition. To change the definition of a function in CREATE TRIGGER syntax, trigger needs to be dropped first before creating it again with new definition, too! So, we need to add the optional clause 'OR REPLACE'. Adding the optional clauses 'AS' and 'OR REPLACE' in CREATE TRIGGER syntax gives the comfort of defining the trigger or redefining the trigger definition which contains the function definition in single command. Here is the syntax based on the previous discussion. CREATE [ OR REPLACE ] [ CONSTRAINT ] TRIGGER name { BEFORE | AFTER | INSTEAD OF } { event [ OR ... ] } ON table_name [ FROM referenced_table_name ] [ NOT DEFERRABLE | [ DEFERRABLE ] { INITIALLY IMMEDIATE | INITIALLY DEFERRED} ] [ FOR [ EACH ] { ROW | STATEMENT } ] [ WHEN ( condition ) ] { EXECUTE PROCEDURE function_name ( arguments) | AS 'trigger function definition' [ LANGUAGE lang_name ] [ SET configuration_parameter { TO value |= value | FROM CURRENT }] } If you have your opinion on this concept, please give me it. Regards, Okano Naoki Fujitsu
"Okano, Naoki" <okano.naoki@jp.fujitsu.com> writes: > I would like to add the following support for a trigger. > This implementation enables to create a trigger efficiently > in single command. > It had been discussed before. The link is as shown below. > https://www.postgresql.org/message-id/CAA-aLv4m%3Df9cc1zcUzM49pE8%2B2NpytUDraTgfBmkTOkMN_wO2w%40mail.gmail.com I think people pretty much lost interest in that proposal, which is why it never got finished. Aside from the definitional issues discussed in that thread, I can think of a few other problems: 1. The impression I have is that most people write trigger functions so that they can be shared across multiple uses. That'd be impossible with anonymous trigger function blocks. So you'd be trading off merging two commands into one, versus having to write out the whole body of the trigger multiple times, which wouldn't be much of a win. 2. I am concerned that every time we add any syntax to CREATE FUNCTION, we'll have to think about whether we need to add it to CREATE TRIGGER. (Or more likely, we'll forget and then users will complain.) 3. There's a lot of infrastructure that's built up around CREATE FUNCTION, such as psql's \ef and \sf commands. We'd soon find ourselves having to duplicate all that for triggers. So personally I feel that the value-for-work-expended ratio here is pretty low. But in any case it would be a serious mistake to do this without first implementing CREATE OR REPLACE TRIGGER. I think that's an entirely separate proposal and you would be well advised to treat it as such. It's far from trivial because of locking considerations. You probably have to take an exclusive lock on the owning relation in order to change trigger properties. (An advantage of the separate-function-definition approach is that you can replace the function body with a much weaker lock.) > Supporting 'AS' clause in CREATE TRIGGER syntax will enable the option of > defining the trigger in single command. > As a bonus, it will be compatible with oracle. It certainly would not match Oracle's syntax for trigger bodies. It might slightly reduce the conversion pain, but only slightly. regards, tom lane
> But in any case it would be a serious mistake to do this without first > implementing CREATE OR REPLACE TRIGGER. I think that's an entirely separate > proposal and you would be well advised to treat it as such. I see. There are more problems than I expected... Let me start with 'OR REPLACE' clause. At least, adding only 'OR REPLACE' clause has the following advantages. * It enables users to redefine a trigger in single command. * The trigger can always be referenced when redefining a trigger. # But it is not so when using 'DROP' and 'CREATE' commands.It is useful when users change the function or change the condition of 'WHEN' clause. Regard, Okano Naoki Fujitsu
On Wednesday, November 16, 2016 4:31 PM Okano Naoki wrote:
> > But in any case it would be a serious mistake to do this without first
> > implementing CREATE OR REPLACE TRIGGER. I think that's an entirely separate
> > proposal and you would be well advised to treat it as such.
> I see. There are more problems than I expected...
> Let me start with 'OR REPLACE' clause.
I tried to cretae a patch for CREATE OR REPLACE TRIGGER.
An example of execution is shown below.
example)
1.define a new trigger
CREATE TRIGGER regular_trigger
AFTER UPDATE ON table_name
REFERENCING OLD TABLE AS oldtable_1 NEW TABLE AS newtable_1
FOR EACH STATEMENT
EXECUTE PROCEDURE func_1();
2.redinfe a trigger in single command
CREATE OR REPLACE TRIGGER regular_trigger
AFTER UPDATE OR DELETE ON table_name
REFERENCING OLD TABLE AS oldtable_2 NEW TABLE AS newtable_2
FOR EACH ROW
EXECUTE PROCEDURE func_2();
If the named trigger does not exist.
a new trigger is also created by using OR REPLACE clause.
A regular trigger cannot be replaced by a constraint trigger and vice varsa.
because a constraint trigger has a different role from regular triger.
Please give me feedback.
Regards,
Okano Naoki
Fujitsu
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On Tue, Feb 7, 2017 at 3:11 AM, Okano, Naoki <okano.naoki@jp.fujitsu.com> wrote: > On Wednesday, November 16, 2016 4:31 PM Okano Naoki wrote: >> > But in any case it would be a serious mistake to do this without first >> > implementing CREATE OR REPLACE TRIGGER. I think that's an entirely separate >> > proposal and you would be well advised to treat it as such. >> I see. There are more problems than I expected... >> Let me start with 'OR REPLACE' clause. > I tried to cretae a patch for CREATE OR REPLACE TRIGGER. > An example of execution is shown below. > > example) > 1.define a new trigger > CREATE TRIGGER regular_trigger > AFTER UPDATE ON table_name > REFERENCING OLD TABLE AS oldtable_1 NEW TABLE AS newtable_1 > FOR EACH STATEMENT > EXECUTE PROCEDURE func_1(); > 2.redinfe a trigger in single command > CREATE OR REPLACE TRIGGER regular_trigger > AFTER UPDATE OR DELETE ON table_name > REFERENCING OLD TABLE AS oldtable_2 NEW TABLE AS newtable_2 > FOR EACH ROW > EXECUTE PROCEDURE func_2(); > > If the named trigger does not exist. > a new trigger is also created by using OR REPLACE clause. > A regular trigger cannot be replaced by a constraint trigger and vice varsa. > because a constraint trigger has a different role from regular triger. > > Please give me feedback. You should add this to the next CommitFest so it doesn't get missed. https://commitfest.postgresql.org/ -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thursday, February 9, 2017 4:41 AM Robert Haas wrote: > You should add this to the next CommitFest so it doesn't get missed. > > https://commitfest.postgresql.org/ Thank you for your kind advice! I have added this patch to the CommitFest 2017-03. Regards, Okano Naoki Fujitsu
On 11/14/16 8:51 PM, Tom Lane wrote: > 1. The impression I have is that most people write trigger functions > so that they can be shared across multiple uses. That'd be impossible > with anonymous trigger function blocks. So you'd be trading off merging > two commands into one, versus having to write out the whole body of the > trigger multiple times, which wouldn't be much of a win. I've fairly frequently needed one-off triggers, usually to do some kind of data validation that wasn't suitable in a CHECK constraint. Enough so that twice now[1] I've created generic trigger functions that allow you to specify an arbitrary expression to test against NEW and OLD. 1: Second, WIP example: https://github.com/decibel/tg_sanity/blob/master/sql/tg_sanity.sql -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com 855-TREBLE2 (855-873-2532)
On 2/7/17 03:11, Okano, Naoki wrote: > I tried to cretae a patch for CREATE OR REPLACE TRIGGER. I have a feeling that this was proposed a few times in the ancient past but did not go through because of locking issues. I can't find any emails about it through. Does anyone remember? Have you thought about locking issues? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut wrote: > I have a feeling that this was proposed a few times in the ancient past > but did not go through because of locking issues. I can't find any > emails about it through. Does anyone remember? Have you thought about > locking issues? Is this e-mail you are finding? https://www.postgresql.org/message-id/20140916124537.GH25887%40awork2.anarazel.de I am considering to add 'OR REPLACE' clause as a first step. At least, I think there is no need to change the locking level when replacing a trigger with 'EXECUTE PROCEDURE' clause. In PostgreSQL, we currently have ShareRowExclusiveLock lock on relation on which trigger is created. ShareRowExclusiveLockis enough to replace a trigger. Also, we currently have RowExclusiveLock on pg_trigger. RowExclusiveLock is enough to replace a trigger, too. Regards, Okano Naoki Fujitsu
On 3/8/17 04:12, Okano, Naoki wrote: > Peter Eisentraut wrote: >> I have a feeling that this was proposed a few times in the ancient past >> but did not go through because of locking issues. I can't find any >> emails about it through. Does anyone remember? Have you thought about >> locking issues? > Is this e-mail you are finding? > https://www.postgresql.org/message-id/20140916124537.GH25887%40awork2.anarazel.de No, that's not the one I had in mind. > I am considering to add 'OR REPLACE' clause as a first step. > At least, I think there is no need to change the locking level when replacing a trigger with 'EXECUTE PROCEDURE' clause. > In PostgreSQL, we currently have ShareRowExclusiveLock lock on relation on which trigger is created. ShareRowExclusiveLockis enough to replace a trigger. > Also, we currently have RowExclusiveLock on pg_trigger. RowExclusiveLock is enough to replace a trigger, too. I'm not saying it's not correct. I was just wondering. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut wrote: > On 3/8/17 04:12, Okano, Naoki wrote: > > Peter Eisentraut wrote: > >> I have a feeling that this was proposed a few times in the ancient past > >> but did not go through because of locking issues. I can't find any > >> emails about it through. Does anyone remember? Have you thought about > >> locking issues? > > Is this e-mail you are finding? > > https://www.postgresql.org/message-id/20140916124537.GH25887%40awork2.anarazel.de > > No, that's not the one I had in mind. I see. But I could only find it. Would anyone know e-mails discussed about locking issues? > > I am considering to add 'OR REPLACE' clause as a first step. > > At least, I think there is no need to change the locking level when replacing a trigger with 'EXECUTE PROCEDURE' clause. > > In PostgreSQL, we currently have ShareRowExclusiveLock lock on relation on which trigger is created. ShareRowExclusiveLockis enough to replace a trigger. > > Also, we currently have RowExclusiveLock on pg_trigger. RowExclusiveLock is enough to replace a trigger, too. > > I'm not saying it's not correct. I was just wondering. Thank you for your opinion! I think we do not need to change locking level in this case. If someone notice a mistake in my understanding, please point out it. Regards, Okano Naoki Fujitsu
"Okano, Naoki" <okano.naoki@jp.fujitsu.com> writes: > Peter Eisentraut wrote: >>> I have a feeling that this was proposed a few times in the ancient past >>> but did not go through because of locking issues. I can't find any >>> emails about it through. Does anyone remember? Have you thought about >>> locking issues? >>> Is this e-mail you are finding? >>> https://www.postgresql.org/message-id/20140916124537.GH25887%40awork2.anarazel.de >> No, that's not the one I had in mind. That's still a thread well worth studying in detail. It does touch on locking issues, in that it points out that we allow you to replace a trigger function's body with CREATE OR REPLACE FUNCTION with no lock at all on the relation(s) it's a trigger for. Even with very lax assumptions about what lock level CREATE OR REPLACE TRIGGER needs, it can't match "none". Now you could certainly argue that we're not being very safe by allowing trigger functions to be changed that way, but that's the current state of affairs. Another thread that you really need to absorb in its entirety is https://www.postgresql.org/message-id/flat/5447578C.2050807%40proxel.se and you might also want to read the older threads that Robert Haas links to early in that thread. The locking-related point that strikes me most forcefully in that thread is the concerns about whether a concurrent pg_dump run will produce a consistent view of the table's triggers. This is problematic mainly because pg_dump itself will see only the catalog entries that were current when it started, but it relies heavily on ruleutils.c which will tend to see committed changes immediately. Now, the existing behavior here is probably not at all perfect, but that doesn't mean it's okay to make things worse with CREATE OR REPLACE TRIGGER. A conservative conclusion would be that C.O.R.T. needs to take AccessExclusiveLock so that it can't run in parallel with pg_dump. Maybe that can be relaxed but it requires some study. (CREATE TRIGGER doesn't have this issue because pg_dump wouldn't see the new trigger at all and thus would never ask ruleutils about it.) Even if you don't care about pg_dump, I think that the question of whether concurrent DML operations would always see (and act upon) instantaneously- consistent versions of a table's trigger state is worth worrying about. regards, tom lane
I am sending the review of this patch
I found the following
v Use <optional> tage in documentation
v Don’t modified existing test case add new one instead
v Comment in pg_constraint.c is extended make it short
v Error message can be more guider if it tells about general rule
v Wrong result in concurrency case
Step to produce the result
1. build with with --enable-cassert and with -DCATCACHE_FORCE_RELEASE=1
2. Run these commands as setup:
CREATE TABLE my_table1 (id integer, name text);
CREATE TABLE my_table2 (id integer);
3. CREATE FUNCTION my_deleteproc1() RETURNS trigger AS $$
begin
DELETE FROM my_table2 WHERE id=OLD.id;
RETURN NULL;
end;$$ LANGUAGE plpgsql;
4. INSERT INTO my_table1 VALUES(323, 'Alex');
INSERT INTO my_table1 VALUES(23, 'Teddy');
INSERT INTO my_table1 VALUES(38, 'Bob');
INSERT INTO my_table2 VALUES(323);
INSERT INTO my_table2 VALUES(23);
INSERT INTO my_table2 VALUES(38);
5. CREATE OR REPLACE TRIGGER my_regular_trigger AFTER DELETE ON my_table1
FOR EACH ROW
EXECUTE PROCEDURE my_deleteproc1();
6. Attach a debugger to your session set a breakpoint at ExecARDeleteTriggers
7. Run this in your session
DELETE FROM my_table1 WHERE id=323;
8. start another session and run:
CREATE OR REPLACE TRIGGER my_regular_trigger AFTER INSERT ON my_table1
FOR EACH ROW
EXECUTE PROCEDURE my_deleteproc1();
9. exite the debugger to release the first session
and the result
postgres=# SELECT * FROM my_table1;
id | name
----+-------
23 | Teddy
38 | Bob
(2 rows)
postgres=# SELECT * FROM my_table2;
id
-----
323
23
38
(3 rows)
Id number 323 should not be there in my_table2;
Regards
Surafel
Hi Naoki, On 3/17/17 11:58 AM, Surafel Temesgen wrote: > I am sending the review of this patch This thread has been idle for a week. Please respond and/or post a new patch by 2017-03-28 00:00 AoE (UTC-12) or this submission will be marked "Returned with Feedback". -- -David david@pgmasters.net
Hi Surafel, Thank you for your review! But I have not finish fixing a patch yet. > v Use <optional> tage in documentation ok. > v Don’t modified existing test case add new one instead These existing tests I modified are the results of commands "SELECT pg_get_triggerdef(oid, true) ... ". I modified them in order to match with pg_get_functiondef(). The keyword 'OR REPLACE' is always added in CREATE FUNCTION. Does not it make sense? > v Comment in pg_constraint.c is extended make it short > v Error message can be more guider if it tells about general rule ok. > v Wrong result in concurrency case Thank you for the detailed example case. I appreciate it. The issue does not seem to be resolved easily because I have to modify pg_get_constraintdef_worker() in ruleutils.c. And it takes some more time to modify pg_get_constraintdef_worker(). So, I am moving this patch to "Returned with Feedback". Regards, Okano Naoki Fujitsu