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

Hello Osumi-san.

I have checked the v15 patch with regard to my v14 review comments.

On Wed, Nov 4, 2020 at 2:53 PM osumi.takamichi@fujitsu.com
<osumi.takamichi@fujitsu.com> 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 ?

Yes, it is good now, apart from some typos in the first sentence:
"grammer" --> "grammar"
"exisiting" --> "existing"

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

Yes, OK. But it might be simpler still just to it like:
"CREATE OR REPLACE TRIGGER works only for replacing a regular (not
constraint) trigger with another regular 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).
> 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.

OK, I understand now. In my v14 review I failed to notice that you did
it this way, which is why I thought a check was missing in the code.

I do think this is a bit subtle. Perhaps this should be asserted and
commented a bit more in the code to make it much clearer what you did.
For example:
----------
BEFORE
/*
 * can't replace constraint trigger.
 */
 if (OidIsValid(existing_constraint_oid))
AFTER
/*
 * It is not allowed to replace with a constraint trigger.
 * The OR REPLACE syntax is not available for constraint triggers (see gram.y).
 */
Assert(!stmt->isconstraint);

/*
 * It is not allowed to replace an existing constraint trigger.
 */
 if (OidIsValid(existing_constraint_oid))
----------


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

Yes, I agree that all the combinations should be present. That is why
I wrote the "create constraint trigger" should be written "create OR
REPLACE constraint trigger" because otherwise AFAIK there is no test
attempting to replace using a constraint trigger - you are only really
testing you cannot create a duplicate name trigger (but those tests
already existed)

In other words, IMO the "incompatible" tests should be like below (I
added comments to try make it more clear what are the combinations)
----------
create or replace trigger my_trig
 after insert on my_table
 for each row execute procedure funcA(); -- 1. create a regular trigger. OK
create or replace constraint trigger my_trig
 after insert on my_table
 for each row execute procedure funcB(); -- Test 1a. Replace regular
trigger with constraint trigger. Expect ERROR (bad syntax)
drop trigger my_trig on my_table;
create constraint trigger my_trig -- 2. create a constraint trigger. OK
 after insert on my_table
 for each row execute procedure funcA();
create or replace trigger my_trig
 after insert on my_table
 for each row execute procedure funcB(); -- Test 2a. Replace
constraint trigger with regular trigger. Expect ERROR (cannot replace
a constraint trigger)
create or replace constraint trigger my_trig
 after insert on my_table
 for each row execute procedure funcB(); -- Test 2b. Replace
constraint trigger with constraint trigger. Expect ERROR (bad syntax)
drop table my_table;
drop function funcA();
drop function funcB();
----------

Kind Regards,
Peter Smith.
Fujitsu Australia