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

Hello Osumi-san.

Below are my v14 patch review comments for your consideration.

===

(1) COMMENT
File: NA
Maybe next time consider using format-patch to make the patch. Then
you can include a comment to give the background/motivation for this
change.

===

(2) COMMENT
File: doc/src/sgml/ref/create_trigger.sgml
@@ -446,6 +454,13 @@ UPDATE OF <replaceable>column_name1</replaceable>
[, <replaceable>column_name2</
Currently it says:
When replacing an existing trigger with CREATE OR REPLACE TRIGGER,
there are restrictions. You cannot replace constraint triggers. That
means it is impossible to replace a regular trigger with a constraint
trigger and to replace a constraint trigger with another constraint
trigger.

--

Is that correct wording? I don't think so. Saying "to replace a
regular trigger with a constraint trigger" is NOT the same as "replace
a constraint trigger".

Maybe I am mistaken but I think the help and the code are no longer in
sync anymore. e.g. In previous versions of this patch you used to
verify replacement trigger kinds (regular/constraint) match. AFAIK you
are not doing that in the current code (but you should be). So
although you say "impossible to replace a regular trigger with a
constraint trigger" I don't see any code to check/enforce that ( ?? )

IMO when you simplified this v14 patch you may have removed some extra
trigger kind conditions that should not have been removed.

Also, the test code should have detected this problem, but I think the
tests have also been broken in v14. See later COMMENT (9).

===

(3) COMMENT
File: src/backend/commands/trigger.c
@@ -185,6 +185,10 @@ CreateTrigger(CreateTrigStmt *stmt, const char
*queryString,
+ bool internal_trigger = false;

--

There is potential for confusion of "isInternal" versus
"internal_trigger". The meaning is not apparent from the names, but
IIUC isInternal seems to be when creating an internal trigger, whereas
internal_trigger seems to be when you found an existing trigger that
was previously created as isInternal.

Maybe something like "existing_isInternal" would be a better name
instead of "internal_trigger".

===

(4) COMMENT
File: src/backend/commands/trigger.c
@@ -185,6 +185,10 @@ CreateTrigger(CreateTrigStmt *stmt, const char
*queryString,
+ bool is_update = false;

Consider if "was_replaced" might be a better name than "is_update".

===

(5) COMMENT
File: src/backend/commands/trigger.c
@@ -669,6 +673,81 @@ CreateTrigger(CreateTrigStmt *stmt, const char
*queryString,
+ if (!stmt->replace)
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("trigger \"%s\" for relation \"%s\" already exists",
+ stmt->trigname, RelationGetRelationName(rel))));
+ else
+ {
+ /*
+ * An internal trigger cannot be replaced by another user defined
+ * trigger. This should exclude the case that internal trigger is
+ * child trigger of partition table and needs to be rewritten when
+ * the parent trigger is replaced by user.
+ */
+ if (internal_trigger && isInternal == false && in_partition == false)
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("trigger \"%s\" for relation \"%s\" is an internal trigger",
+ stmt->trigname, RelationGetRelationName(rel))));
+
+ /*
+ * CREATE OR REPLACE TRIGGER command can't replace constraint
+ * trigger.
+ */
+ if (OidIsValid(existing_constraint_oid))
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("trigger \"%s\" for relation \"%s\" is a constraint trigger",
+ stmt->trigname, RelationGetRelationName(rel))));
+ }

It is not really necessary for the "OR REPLACE" code to need to be
inside an "else" block like that because the "if (!stmt->replace)" has
already been tested above. Consider removing the "else {" to remove
unnecessary indent if you want to.

===

(6) COMMENT
File: src/backend/commands/trigger.c
(same code block as above)

Condition is strangely written:
e.g.
Before: if (internal_trigger && isInternal == false && in_partition == false)
After: if (internal_trigger && !isInternal && !in_partition)

===

(7) COMMENT
File: src/backend/commands/trigger.c
(same code block as above)
/*
 * CREATE OR REPLACE TRIGGER command can't replace constraint
 * trigger.
 */

--

Only need to say
/* Can't replace a constraint trigger. */

===

(8) COMMENT
File: src/include/nodes/parsenodes.h
@@ -2429,6 +2429,9 @@ typedef struct CreateAmStmt

The comment does not need to say "when true,". Just saying "replace
trigger if already exists" is enough.

===

(9) COMMENT
File: src/test/regress/expected/triggers.out
+-- test for detecting incompatible replacement of trigger
+create table my_table (id integer);
+create function funcA() returns trigger as $$
+begin
+  raise notice 'hello from funcA';
+  return null;
+end; $$ language plpgsql;
+create function funcB() returns trigger as $$
+begin
+  raise notice 'hello from funcB';
+  return null;
+end; $$ language plpgsql;
+create or replace trigger my_trig
+  after insert on my_table
+  for each row execute procedure funcA();
+create constraint trigger my_trig
+  after insert on my_table
+  for each row execute procedure funcB(); -- should fail
+ERROR:  trigger "my_trig" for relation "my_table" already exists

--

I think this test has been broken in v14. That last "create constraint
trigger my_trig" above can never be expected to work simply because
you are not specifying the "OR REPLACE" syntax. So in fact this is not
properly testing for incompatible types at all. It needs to say
"create OR REPLACE constraint trigger my_trig" to be testing what it
claims to be testing.

I also think there is a missing check in the code - see COMMENT (2) -
for handling this scenario. But since this test case is broken you do
not then notice the code check is missing.

===

Kind Regards,
Peter Smith.
Fujitsu Australia