Re: extension patch of CREATE OR REPLACE TRIGGER

On Mon, Aug 24, 2020 at 9:33 PM osumi(dot)takamichi(at)fujitsu(dot)com
<osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
> I've fixed my v06 and created v07.

Hi Osumi-san.

I have reviewed the test code of the v07 patch.

Below are my comments.

====

COMMENT (confusing functions)

+create function before_replacement() returns trigger as $$
+begin
+raise notice 'function replaced by another function';
+return null;
+end; $$ language plpgsql;
+create function after_replacement() returns trigger as $$
+begin
+raise notice 'function to replace the initial function';
+return null;
+end; $$ language plpgsql;

Why have function names with a hard-wired dependency on how you expect
they will be called.
I think just call them "funcA" and "funcB" is much easier and works
just as well. e.g.
---
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;
---

And this same comment applies for all the other test functions created
for this v07 patch.

====

COMMENT (drops)

+-- setup for another test of CREATE OR REPLACE TRIGGER
+drop table if exists parted_trig;
+NOTICE: table "parted_trig" does not exist, skipping
+drop trigger if exists my_trig on parted_trig;
+NOTICE: relation "parted_trig" does not exist, skipping
+drop function if exists before_replacement;
+NOTICE: function before_replacement() does not exist, skipping
+drop function if exists after_replacement;
+NOTICE: function after_replacement() does not exist, skipping

Was it deliberate to attempt to drop the trigger after dropping the table?
Also this seems to be dropping functions which were already dropped
just several lines earlier.

====

COMMENT (typos)

There are a couple of typos in the test comments. e.g.
"vefify" -> "verify"
"child parition" -> "child partition"

====

COMMENT (partition table inserts)

1. Was it deliberate to insert explicitly into each partition table?
Why not insert everything into the top table and let the partitions
take care of themselves?

2. The choice of values to insert also seemed strange. Inserting 1 and
1 and 10 is going to all end up in the "parted_trig_1_1".

To summarise, I thought all subsequent partition tests maybe should be
inserting more like this:
---
insert into parted_trig (a) values (50); -- into parted_trig_1_1
insert into parted_trig (a) values (1500); -- into parted_trig_2
insert into parted_trig (a) values (2500); -- into default_parted_trig
---

====

COMMENT (missing error test cases)

There should be some more test cases to cover the new error messages
that were added to trigger.c:

e.g. test for "can't create regular trigger because already exists"
e.g. test for "can't create constraint trigger because already exists"
e.g. test for "can't replace regular trigger with constraint trigger""
e.g. test for "can't replace constraint trigger with regular trigger"
etc.

====

COMMENT (trigger with pending events)

This is another test where the complexity of the functions
("not_replaced", and "fail_to_replace") seemed excessive.
I think just calling these "funcA" and "funcB" as mentioned above
would be easier, and would serve just as well.

====

Kind Regards,
Peter Smith.
Fujitsu Australia