From: | "osumi(dot)takamichi(at)fujitsu(dot)com" <osumi(dot)takamichi(at)fujitsu(dot)com> |
---|---|
To: | 'Peter Smith' <smithpb2250(at)gmail(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-08 11:36:10 |
Message-ID: | OSBPR01MB4888BD46263BBFAC476BD5DBED290@OSBPR01MB4888.jpnprd01.prod.outlook.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | 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
Attachment | Content-Type | Size |
---|---|---|
CREATE_OR_REPLACE_TRIGGER_v09.patch | application/octet-stream | 40.1 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Fujii Masao | 2020-09-08 11:48:16 | Re: Global snapshots |
Previous Message | Jakub Wartak | 2020-09-08 11:17:08 | Re: Division in dynahash.c due to HASH_FFACTOR |