RE: extension patch of CREATE OR REPLACE TRIGGER

From: "osumi(dot)takamichi(at)fujitsu(dot)com" <osumi(dot)takamichi(at)fujitsu(dot)com>
To: 'Peter Smith' <smithpb2250(at)gmail(dot)com>
Cc: "tsunakawa(dot)takay(at)fujitsu(dot)com" <tsunakawa(dot)takay(at)fujitsu(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Michael Paquier <michael(at)paquier(dot)xyz>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, Surafel Temesgen <surafel3000(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: RE: extension patch of CREATE OR REPLACE TRIGGER
Date: 2020-11-06 08:06:14
Message-ID: OSBPR01MB488816D190A3ED550BF55658EDED0@OSBPR01MB4888.jpnprd01.prod.outlook.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hello

On Friday, November 6, 2020 2:25 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > > 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.
The simpler, the better for sure ? I deleted that 2nd sentence.

> > > > > (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
This was not a mistake. The cases of 40 lines are with OR REPLACE to define
each regular trigger that will be overwritten.
But, it doesn't make nothing probably so I deleted such cases.
Please forget that part.

> 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.
Sigh. Yeah, those were not right. Fixed.

>
> 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.
I cannot thank you enough.

Best,
Takamichi Osumi

Attachment Content-Type Size
CREATE_OR_REPLACE_TRIGGER_v17.patch application/octet-stream 30.3 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Kyotaro Horiguchi 2020-11-06 08:24:35 Re: Protect syscache from bloating with negative cache entries
Previous Message Michael Paquier 2020-11-06 07:34:34 Refactor MD5 implementations and switch to EVP for OpenSSL