Re: extension patch of CREATE OR REPLACE TRIGGER - Mailing list 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