Re: extension patch of CREATE OR REPLACE TRIGGER

Hello Osumi-san.

I have checked again v16 patch w.r.t. to my previous comments.

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

Actually, I meant that after making that 1st sentence wording change,
I thought the 2nd sentence (i.e. "That means it is impossible...") is
no longer needed at all since it is just re-stating what the 1st
sentence already says.

But if you prefer to leave it worded how it is now that is ok too.

> > > > (9) COMMENT
(snip)
> 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.

Yes, the tests are generally OK, but unfortunately a few new problems
are introduced with the refactoring of the combination tests.

1) It looks like about 40 lines of test code are cut/paste 2 times by accident
2) Typo "gramatically" --> "grammatically"
3) Your last test described as "create or replace constraint trigger
is not gramatically correct." is not really doing what it is meant to
do. That test was supposed to be trying to replace an existing
CONSTRAINT trigger.

IMO if all the combination tests were consistently commented like my 8
examples below then risk of accidental mistakes is reduced.
e.g.
-- 1. Overwrite existing regular trigger with regular trigger (without
OR REPLACE)
-- 2. Overwrite existing regular trigger with regular trigger (with OR REPLACE)
-- 3. Overwrite existing regular trigger with constraint trigger
(without OR REPLACE)
-- 4. Overwrite existing regular trigger with constraint trigger (with
OR REPLACE)
-- 5. Overwrite existing constraint trigger with regular trigger
(without OR REPLACE)
-- 6. Overwrite existing constraint trigger with regular trigger (with
OR REPLACE)
-- 7. Overwrite existing constraint trigger with constraint trigger
(without OR REPLACE)
-- 8. Overwrite existing constraint trigger with constraint trigger
(with OR REPLACE)

To avoid any confusion I have attached triggers.sql updated how I
think it should be. Please compare it to see what I mean. PSA.

I hope it helps.

===

Kind Regards,
Peter Smith.
Fujitsu Australia