RE: extension patch of CREATE OR REPLACE TRIGGER

Hi

Peter-San, thanks for your support.
On Monday, November 2, 2020 2:39 PM Peter Smith wrote:
> ===
>
> (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.
OK. How about v15 ?

> ===
>
> (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".
I corrected my wording in create_trigger.sgml, which should cause less confusion
than v14. The reason why I changed the documents is described below.

> 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).
Don't worry and those are not broken.

I changed some codes in gram.y to throw a syntax error when
OR REPLACE clause is used with CREATE CONSTRAINT TRIGGER.

In the previous discussion with Tsunakawa-San in this thread,
I judged that OR REPLACE clause is
not necessary for *CONSTRAINT* TRIGGER to achieve the purpose of this patch.
It is to support the database migration from Oracle to Postgres
by supporting the same syntax for trigger replacement. Here,
because the constraint trigger is unique to the latter,
I prohibited the usage of CREATE CONSTRAINT TRIGGER and OR REPLACE
clauses at the same time at the grammatical level.
Did you agree with this way of modification ?

To prohibit the combination OR REPLACE and CONSTRAINT clauses may seem
a little bit radical but I refer to an example of the combination to use
CREATE CONSTRAINT TRIGGER and AFTER clause.
When the timing of trigger is not AFTER for CONSTRAINT TRIGGER,
an syntax error is caused. I learnt and followed the way for
my modification from it.

> ===
>
> (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".
Definitely sounds better. I fixed the previous confusing name.

>
> ===
>
> (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".
>
> ===
Also, this must be good. Done.

>
> (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.
Yeah, you are right. Fixed.

> ===
>
> (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)
OK. Done

>
> ===
>
> (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.
>
> ===
Applied.

> (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.
As I described above, the grammatical error occurs to use
"CREATE OR REPLACE CONSTRAINT TRIGGER" in v14 (and v15 also).
At the time to write v14, I wanted to list up all imcompatible cases
even if some tests did *not* or can *not* contain "OR REPLACE" clause.
I think this way of change seemed broken to you.

Still now I think it's a good idea to cover such confusing cases,
so I didn't remove both failure tests in v15
(1) CREATE OR REPLACE TRIGGER creates a regular trigger and execute
CREATE CONSTRAINT TRIGGER, which should fail
(2) CREATE CONSTRAINT TRIGGER creates a constraint trigger and
execute CREATE OR REPLACE TRIGGER, which should fail
in order to show in such cases, the detection of error nicely works.
The results of tests are fine.

> 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.
>
> ===
My inappropriate explanation especially in the create_trigger.sgml
made you think those are broken. But, as I said they are necessary still
to cover corner combination cases.

Best,
Takamichi Osumi