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
