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

Hi,


On Thursday, November 5, 2020 1:22 PM
Peter Smith <smithpb2250@gmail.com> wrote:
> 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"
Sorry for such minor mistakes. Fixed.


> > > ===
> > >
> > > (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."
Yeah, this kind of supplementary words help user to understand the
exact usage of this feature. Thanks.

> 
> >
> > > 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))
> ----------
Agreed.
Note that this part of the latest patch v16 shows different indent of the comments
that you gave me in your previous reply but those came from the execution of pgindent.

> 
> > > (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();
> ----------
I understand that
I need to add 2 syntax error cases and
1 error case to replace constraint trigger at least. It makes sense.
At the same time, I supposed that the order of the tests
in v15 patch is somehow hard to read.
So, I decided to sort out those and take your new sets of tests there.
What I'd like to test there is not different, though.
Please have a look at the new patch.


Best,
    Takamichi Osumi