RE: extension patch of CREATE OR REPLACE TRIGGER

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