From: | Peter Smith <smithpb2250(at)gmail(dot)com> |
---|---|
To: | "osumi(dot)takamichi(at)fujitsu(dot)com" <osumi(dot)takamichi(at)fujitsu(dot)com> |
Cc: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Michael Paquier <michael(at)paquier(dot)xyz>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, Surafel Temesgen <surafel3000(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: extension patch of CREATE OR REPLACE TRIGGER |
Date: | 2020-09-03 07:35:36 |
Message-ID: | CAHut+PudUi7z3t-oDZcs7FwSFCu5+WHiD+kL6DzOXCFJ_02FPQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi Osumi-san.
Thanks for addressing my previous review comments in your new v08 patch.
I have checked it again.
My only remaining comments are for trivial stuff:
====
COMMENT trigger.c (tab alignment)
@@ -184,6 +185,13 @@ CreateTrigger(CreateTrigStmt *stmt, const char
*queryString,
char *oldtablename = NULL;
char *newtablename = NULL;
bool partition_recurse;
+ bool is_update = false;
+ HeapTuple newtup;
+ TupleDesc tupDesc;
+ bool replaces[Natts_pg_trigger];
+ Oid existing_constraint_oid = InvalidOid;
+ bool trigger_exists = false;
+ bool trigger_deferrable = false;
Maybe my editor configuration is wrong, but the alignment of
"existing_constraint_oid" still does not look fixed to me.
====
COMMENT trigger.c (move some declarations)
>> 2. Maybe it is more convenient/readable to declare some of these in the scope
>> where they are actually used.
>> e.g. newtup, tupDesc, replaces.
>I cannot do this because those variables are used
>at the top level in this function. Anyway, thanks for the comment.
Are you sure it can't be done? It looks doable to me.
====
COMMENT trigger.c ("already exists" message)
In my v07 review I suggested adding a hint for the new "already exists" error.
Of course choice is yours, but since you did not add it I just wanted
to ask was that accidental or deliberate?
====
COMMENT triggers.sql/.out (typo in comment)
+-- test for detecting imcompatible replacement of trigger
"imcompatible" -> "incompatible"
====
COMMENT triggers.sql/.out (wrong comment)
+drop trigger my_trig on my_table;
+create or replace constraint trigger my_trig
+ after insert on my_table
+ for each row execute procedure funcB(); --should fail
+create or replace trigger my_trig
+ after insert on my_table
+ for each row execute procedure funcB(); --should fail
+ERROR: trigger "my_trig" for relation "my_table" is a constraint trigger
+HINT: use CREATE OR REPLACE CONSTRAINT TRIGGER to replace a costraint trigger
I think the first "--should fail" comment is misleading. First time is OK.
====
Kind Regards,
Peter Smith.
Fujitsu Australia
From | Date | Subject | |
---|---|---|---|
Next Message | Daniel Gustafsson | 2020-09-03 07:47:07 | Re: Switch to multi-inserts for pg_depend |
Previous Message | Fujii Masao | 2020-09-03 07:05:05 | Re: New statistics for tuning WAL buffer size |