RE: extension patch of CREATE OR REPLACE TRIGGER - Mailing list pgsql-hackers

Hi, Peter-San

I've fixed all except one point.
> My only remaining comments are for trivial stuff:
Not trivial but important.

> 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.
You were right. In order to solve this point completely,
I've executed pgindent and gotten clean alignment.
How about v09 ?
Other alignments of C source codes have been fixed as well.
This is mainly comments, though.

> 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.
Done. I was wrong. Thank you.

> 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?
This was deliberate.
The code path of "already exists" error you mentioned above
is used for other errors as well. For example, a failure case of 
"ALTER TABLE name ATTACH PARTITION partition_name...".

This command fails if the "partition_name" table has a trigger,
whose name is exactly same as the trigger on the "name" table.
For each user-defined row-level trigger that exists in the "name" table,
a corresponding one is created in the attached table, automatically.
Thus, the "ALTER TABLE" command throws the error which says
trigger "name" for relation "partition_name" already exists.
I felt if I add the hint, application developer would get confused.
Did it make sense ?

> COMMENT triggers.sql/.out (typo in comment)
> 
> +-- test for detecting imcompatible replacement of trigger
> 
> "imcompatible" -> "incompatible"
Fixed.


> 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.
Thanks. Removed the misleading comment.


Regards,
    Takamichi Osumi