Thread: extension patch of CREATE OR REPLACE TRIGGER

On Thu, 28 Feb 2019 at 21:44, Osumi, Takamichi
<osumi.takamichi@jp.fujitsu.com> wrote:
> I've made a patch to add CREATE OR REPLACE TRIGGER with some basic tests in triggers.sql.

Hi,

I see there are two patch entries in the commitfest for this.  Is that
a mistake? If so can you "Withdraw" one of them?

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

> I've made a patch to add CREATE OR REPLACE TRIGGER with some basic tests in triggers.sql.

>> I see there are two patch entries in the commitfest for this.  Is that a
>> mistake? If so can you "Withdraw" one of them?

Oh my bad. Sorry, this time was my first time to register my patch !
Please withdraw the old one, "extension patch to add OR REPLACE clause to CREATE TRIGGER".
My latest version is "extension patch of CREATE OR REPLACE TRIGGER".

Thanks ! 
-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

On 2/28/19 10:43 AM, Osumi, Takamichi wrote:
> 
> One past thread about introducing CREATE OR REPLACE TRIGGER into the syntax
> 
> had stopped without complete discussion in terms of LOCK level.
> 
> The past thread is this. I'd like to inherit this one.

Since this patch landed at the last moment in the last commitfest for  
PG12 I have marked it as targeting PG13.

Regards,
-- 
-David
david@pgmasters.net

On Tue, Mar 5, 2019 at 10:19 PM David Steele <david@pgmasters.net> wrote:
> On 2/28/19 10:43 AM, Osumi, Takamichi wrote:
> > One past thread about introducing CREATE OR REPLACE TRIGGER into the syntax
> >
> > had stopped without complete discussion in terms of LOCK level.
> >
> > The past thread is this. I'd like to inherit this one.
>
> Since this patch landed at the last moment in the last commitfest for
> PG12 I have marked it as targeting PG13.

Hello Osumi-san,

The July Commitfest is now beginning.  To give the patch the best
chance of attracting reviewers, could you please post a rebased
version?  The last version doesn't apply.

Thanks,

-- 
Thomas Munro
https://enterprisedb.com

Dear Mr. Thomas

> The July Commitfest is now beginning.  To give the patch the best chance of
> attracting reviewers, could you please post a rebased version?  The last version
> doesn't apply.

I really appreciate your comments.
Recently, I'm very busy because of my work.
So, I'll fix this within a couple of weeks.

Regards,
    Osumi Takamichi

On Wed, Jul 03, 2019 at 04:37:00AM +0000, osumi.takamichi@fujitsu.com wrote:
> I really appreciate your comments.
> Recently, I'm very busy because of my work.
> So, I'll fix this within a couple of weeks.

Please note that I have switched the patch as waiting on author.
--
Michael

Dear Michael san

> > So, I'll fix this within a couple of weeks.
> Please note that I have switched the patch as waiting on author.

Thanks for your support.
I've rebased the previous patch to be applied
to the latest PostgreSQL without any failure of regression tests.

Best,
    Takamichi Osumi

"osumi.takamichi@fujitsu.com" <osumi.takamichi@fujitsu.com> writes:
> [ CREATE_OR_REPLACE_TRIGGER_v03.patch ]

I took a quick look through this just to see what was going on.
A few comments:

* Upthread you asked about changing the lock level to be
AccessExclusiveLock if the trigger already exists, but the patch doesn't
actually do that.  Which is fine by me, because that sounds like a
perfectly bad idea.  In the first place, nobody is going to expect that
OR REPLACE changes the lock level, and in the second place, you can't
actually tell whether the trigger exists until you already have some
lock on the table.  I do not put any credit in the argument that it's
more important to lock out pg_dump against a concurrent REPLACE TRIGGER
than it is to lock out a concurrent CREATE TRIGGER, anyway.  So I think
keeping it at ShareRowExclusiveLock is fine.

* I wouldn't recommend adding CreateConstraintEntry's new argument
at the end.  IME, "add at the end" is almost always bad coding style;
the right thing is "add where it belongs, that is where you'd have
put it if you were writing the list from scratch".  To the admittedly
imperfect extent that the order of CreateConstraintEntry's arguments
matches the column order of pg_constraint, there's a good argument
that the OID should be *first*.  (Maybe, as long as we've gotta touch
all the callers anyway, we should fix the other random deviations
from the catalog's column order, too.)

* While you're at it, it wouldn't hurt to fix CreateConstraintEntry's
header comment, maybe like

- * The new constraint's OID is returned.
+ * The new constraint's OID is returned.  (This will be the same as
+ * "conOid" if that is specified as nonzero.)

* The new code added to CreateTrigger could stand a rethink, too.
For starters, this comment does not describe the code stanza
just below it, but something considerably further down:

     /*
+     * Generate the trigger's OID now, so that we can use it in the name if
+     * needed.
+     */

It's also quite confusing because if there is a pre-existing trigger,
we *don't* generate any new OID.  I'd make that say "See if there is a
pre-existing trigger of the same name", and then comment the later OID
generation appropriately.  Also, the code below the pg_trigger search
seems pretty confused and redundant:

+    if (!trigger_exists)
+        // do something
+    if (stmt->replace && trigger_exists)
+    {
+        if (stmt->isconstraint && !OidIsValid(existing_constraint_oid))
+            //  do something
+        else if (!stmt->isconstraint && OidIsValid(existing_constraint_oid))
+            // do something
+    }
+    else if (trigger_exists && !isInternal)
+    {
+        // do something
+    }

I'm not on board with the idea that testing trigger_exists three separate
times, in three randomly-different-looking ways, makes things more
readable.  I'm also not excited about spending the time to scan pg_trigger
at all in the isInternal case, where you're going to ignore the result.
So I think this could use some refactoring.

Also, in the proposed tests:

+\h CREATE TRIGGER;

We do not test \h output in any existing regression test, and we're
not going to start doing so in this one.  For one thing, the expected
URL would break every time we forked off a new release branch.
(There would surely be value in having more-than-no test coverage
of psql/help.c, but that's a matter for its own patch, which would
need some thought about how to cope with instability of the output.)

            regards, tom lane

On Tue, Jul 30, 2019 at 04:44:11PM -0400, Tom Lane wrote:
> We do not test \h output in any existing regression test, and we're
> not going to start doing so in this one.  For one thing, the expected
> URL would break every time we forked off a new release branch.
> (There would surely be value in having more-than-no test coverage
> of psql/help.c, but that's a matter for its own patch, which would
> need some thought about how to cope with instability of the output.)

One way to get out of that could be some psql-level options to control
which parts of the help output is showing up.  The recent addition of
the URL may bring more weight for doing something in this area.
--
Michael

On Wed, Jul 31, 2019 at 1:33 PM Michael Paquier <michael@paquier.xyz> wrote:
> On Tue, Jul 30, 2019 at 04:44:11PM -0400, Tom Lane wrote:
> > We do not test \h output in any existing regression test, and we're
> > not going to start doing so in this one.  For one thing, the expected
> > URL would break every time we forked off a new release branch.
> > (There would surely be value in having more-than-no test coverage
> > of psql/help.c, but that's a matter for its own patch, which would
> > need some thought about how to cope with instability of the output.)
>
> One way to get out of that could be some psql-level options to control
> which parts of the help output is showing up.  The recent addition of
> the URL may bring more weight for doing something in this area.

Hello again Osumi-san,

The end of CF1 is here.  I've moved this patch to CF2 (September) in
the Commitfest app.  Of course, everyone is free to continue
discussing the patch before then.  When you have a new version, please
set the status to "Needs review".

-- 
Thomas Munro
https://enterprisedb.com

On 2019-Jul-22, osumi.takamichi@fujitsu.com wrote:

> Dear Surafel
> 
> Thank you for your check of my patch.
> I’ve made the version 03 to
> fix what you mentioned about my patch.
> 
> I corrected my wrong bracing styles and
> also reduced the amount of my regression test.
> Off course, I erased unnecessary
> white spaces and the C++ style comment.

A new version of this patch, handling Tom's comments, would be
appreciated.  Besides, per CFbot this patch applies no longer.

Thanks

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Dear Tom Lane

Thank you so much for your comment.

> * Upthread you asked about changing the lock level to be AccessExclusiveLock if
> the trigger already exists, but the patch doesn't actually do that.  Which is fine by
> me, because that sounds like a perfectly bad idea.

Why I suggested a discussion
to make the lock level of C.O.R.T. stronger above comes from my concern.

I've worried about a case that
C.O.R.T. weak lock like ShareRowExclusiveLock allows
one session to replace other session's trigger for new trigger by COMMIT;
As a result, the session is made to use the new one unintentionally.

As you can see below, the previous trigger is replaced by Session2 after applying this patch.
This seems to conflict with user's expectation to data consistency between sessions or
to identify C.O.R.T with DROP TRIGGER (AcessExclusive) + CREATE TRIGGER in terms of lock level.

-- Preparation
create table my_table1 (id integer, name text);
create table my_table2 (id integer, name text);
CREATE OR REPLACE FUNCTION public.my_updateproc1() RETURNS trigger LANGUAGE plpgsql
 AS $function$
  begin
    UPDATE my_table2 SET name = 'new ' WHERE id=OLD.id;
    RETURN NULL;
  end;$function$;

CREATE OR REPLACE FUNCTION public.my_updateproc2() RETURNS trigger LANGUAGE plpgsql
 AS $function$
  begin
    UPDATE my_table2 SET name = 'replace' WHERE id=OLD.id;
    RETURN NULL;
  end;$function$;

CREATE OR REPLACE TRIGGER my_regular_trigger AFTER UPDATE
       ON my_table1 FOR EACH ROW EXECUTE PROCEDURE my_updateproc1();

--Session 1---
BEGIN;
select * from my_table1; -- Cause AccessShareLock here by referring to my_table1;

--Session 2---
BEGIN;
CREATE OR REPLACE TRIGGER my_regular_trigger
   AFTER UPDATE ON my_table1 FOR EACH ROW
     EXECUTE PROCEDURE my_updateproc2();
COMMIT;

--Session 1---
select pg_get_triggerdef(oid, true) from pg_trigger where tgrelid = 'my_table1'::regclass AND tgname =
'my_regular_trigger'; 
------------------------------------------------------------------------------------------------------------
 CREATE TRIGGER my_regular_trigger AFTER UPDATE ON my_table1 FOR EACH ROW EXECUTE FUNCTION my_updateproc2()
(1 row)

By the way, I've fixed other points of my previous patch.
> * I wouldn't recommend adding CreateConstraintEntry's new argument at the end.
I changed the order of CreateConstraintEntry function and its header comment.

Besides that,
> I'm not on board with the idea that testing trigger_exists three separate times, in
> three randomly-different-looking ways, makes things more readable.
I did code refactoring of the redundant and confusing part.

> We do not test \h output in any existing regression test
And off course, I deleted the \h test you mentioned above.

Regards,
       Takamichi Osumi

On Thu, Aug 01, 2019 at 09:49:53PM +1200, Thomas Munro wrote:
> The end of CF1 is here.  I've moved this patch to CF2 (September) in
> the Commitfest app.  Of course, everyone is free to continue
> discussing the patch before then.  When you have a new version, please
> set the status to "Needs review".

The latest patch includes calls to heap_open(), causing its
compilation to fail.  Could you please send a rebased version of the
patch?  I have moved the entry to next CF, waiting on author.
--
Michael

Dear Michael san

> The latest patch includes calls to heap_open(), causing its compilation to fail.
> Could you please send a rebased version of the patch?  I have moved the entry to
> next CF, waiting on author.
Thanks. I've fixed where you pointed out.
Also, I'm waiting for other kind of feedbacks from anyone.

Regards,
    Osumi Takamichi

On 12/1/19 8:56 PM, osumi.takamichi@fujitsu.com wrote:
> 
>> The latest patch includes calls to heap_open(), causing its compilation to fail.
>> Could you please send a rebased version of the patch?  I have moved the entry to
>> next CF, waiting on author.
> Thanks. I've fixed where you pointed out.

This patch no longer applies: http://cfbot.cputube.org/patch_27_2307.log

CF entry has been updated to Waiting on Author.

> Also, I'm waiting for other kind of feedbacks from anyone.

Hopefully a re-based patch will help with that.

Regards,
-- 
-David
david@pgmasters.net

"osumi.takamichi@fujitsu.com" <osumi.takamichi@fujitsu.com> writes:
> Also, I'm waiting for other kind of feedbacks from anyone.

As David pointed out, this needs to be rebased, though it looks like
the conflict is pretty trivial.

A few other notes from a quick look:

* You missed updating equalfuncs.c/copyfuncs.c.  Pretty much any change in
a Node struct will require touching backend/nodes/ functions, and in
general it's a good idea to grep for uses of the struct to see what else
might be affected.

* Did you use a dartboard while deciding where to add the new field
in struct CreateTrigger?  Its placement certainly seems quite random.
Maybe we should put both "replace" and "isconstraint" up near the
front, to match up with the statement's syntax.

* The patch doesn't appear to have any defenses against being asked to
replace the definition of, say, a foreign key trigger.  It might be
sufficient to refuse to replace an entry that has tgisinternal set,
though I'm not sure if that covers all cases that we'd want to disallow.

* Speaking of which, I think you broke the isInternal case by insisting
on doing a lookup first.  isInternal should *not* do a lookup, period,
especially not with the name it's initially given which will not be the
final trigger name.  A conflict on that name is irrelevant, and so is
the OID of any pre-existing trigger.

* I'm not entirely sure that this patch interacts gracefully with
the provisions for per-partition triggers, either.  Is the change
correctly cascaded to per-partition triggers if there are any?
Do we disallow making a change on a child partition trigger rather
than its parent?  (Checking tgisinternal is going to be bound up
in that, since it looks like somebody decided to set that for child
triggers.  I'm inclined to think that that was a dumb idea; we
may need to break out a separate tgischild flag so that we can tell
what's what.)

* I'm a little bit concerned about the semantics of changing the
tgdeferrable/tginitdeferred properties of an existing trigger.  If there
are trigger events pending, and the trigger is redefined in such a way
that those events should already have been fired, what then?  This doesn't
apply in other sessions, because taking ShareRowExclusiveLock should be
enough to ensure that no other session has uncommitted updates pending
against the table.  But it *does* apply in our own session, because
ShareRowExclusiveLock won't conflict against our own locks.  One answer
would be to run CheckTableNotInUse() once we discover that we're modifying
an existing trigger.  Or we could decide that it doesn't matter --- if you
do that and it breaks, tough.  For comparison, I notice that there doesn't
seem to be any guard against dropping a trigger that has pending events
in our own session, though that doesn't work out too well:

regression=# create constraint trigger my_trig after insert on trig_table deferrable initially deferred for each row
executeprocedure before_replacement(); 
CREATE TRIGGER
regression=# begin;
BEGIN
regression=*# insert into trig_table default values;
INSERT 0 1
regression=*# drop trigger my_trig on trig_table;
DROP TRIGGER
regression=*# commit;
ERROR:  relation 38489 has no triggers

But arguably that's a bug to be fixed, not desirable behavior to emulate.

* Not the fault of this patch exactly, but trigger.c seems to have an
annoyingly large number of copies of the code to look up a trigger by
name.  I wonder if we could refactor that, say by extracting the guts of
get_trigger_oid() into an internal function that's passed an already-open
pg_trigger relation.

* Upthread you were complaining about ShareRowExclusiveLock not being a
strong enough lock, but I think that's nonsense, for the same reason that
it's a sufficient lock for plain CREATE TRIGGER: if we have that lock then
no other session can have pending trigger events of any sort on the
relation, nor can new ones get made before we commit.  But there's no
reason to lock out SELECTs on the relation, since those don't interact
with triggers.

            regards, tom lane

Dear Tom Lane

Thanks for your so many fruitful comments !

I have fixed my patch again.
On the other hand, there're some questions left
that I'd like to discuss.

> * You missed updating equalfuncs.c/copyfuncs.c.  Pretty much any change in a
> Node struct will require touching backend/nodes/ functions, and in general it's a
> good idea to grep for uses of the struct to see what else might be affected.
Yeah, thanks.

> * Did you use a dartboard while deciding where to add the new field in struct
> CreateTrigger?  Its placement certainly seems quite random.
> Maybe we should put both "replace" and "isconstraint" up near the front, to match
> up with the statement's syntax.
By following the statement's syntax of CREATE TRIGGER,
I've listed up where I should change and fixed their orders.

> * Speaking of which, I think you broke the isInternal case by insisting on doing a
> lookup first.  isInternal should *not* do a lookup, period, especially not with the
> name it's initially given which will not be the final trigger name.  A conflict on that
> name is irrelevant, and so is the OID of any pre-existing trigger.
Sorry for this.
I inserted codes to skip the first lookup for isInternal case.
As a result, when isInternal is set true, trigger_exists flag never becomes true.
Doing a lookup first is necessary to fetch information
for following codes such as existing_constraint_oid to run CreateConstraintEntry().

> * I'm not entirely sure that this patch interacts gracefully with the provisions for
> per-partition triggers, either.  Is the change correctly cascaded to per-partition
> triggers if there are any?
Yes.
Please check added 4 test cases to prove that replacement of trigger
cascades to partition's trigger when there are other triggers on the relation.

> * The patch doesn't appear to have any defenses against being asked to
> replace the definition of, say, a foreign key trigger.  It might be
> sufficient to refuse to replace an entry that has tgisinternal set,
> though I'm not sure if that covers all cases that we'd want to disallow.

> Do we disallow making a change on a child partition trigger rather than its parent?
> (Checking tgisinternal is going to be bound up in that, since it looks like somebody
> decided to set that for child triggers.  I'm inclined to think that that was a dumb
> idea; we may need to break out a separate tgischild flag so that we can tell what's
> what.)
Does this mean I need to add a new catalog member named 'tgischild' in pg_trigger?
This change sounds really widely influential, which means touching other many files additionally.
Isn't there any other way to distinguish trigger on partition table
from internally generated trigger ?
Otherwise, I need to fix many codes to achieve
the protection of internally generated trigger from being replaced.

> * I'm a little bit concerned about the semantics of changing the
> tgdeferrable/tginitdeferred properties of an existing trigger.  If there are trigger
> events pending, and the trigger is redefined in such a way that those events
> should already have been fired, what then?
OK. I need a discussion about this point.
There would be two ideas to define the behavior of this semantics change, I think.
The first idea is to throw an error that means
the *pending* trigger can't be replaced during the session.
The second one is just to replace the trigger and ignite the new trigger
at the end of the session when its tginitdeferred is set true.
For me, the first one sounds safer. Yet, I'd like to know other opinions.

> regression=# create constraint trigger my_trig after insert on trig_table
> deferrable initially deferred for each row execute procedure
> before_replacement(); CREATE TRIGGER regression=# begin; BEGIN
> regression=*# insert into trig_table default values; INSERT 0 1 regression=*#
> drop trigger my_trig on trig_table; DROP TRIGGER regression=*# commit;
> ERROR:  relation 38489 has no triggers
I could reproduce this bug, using the current master without my patch.
So this is another issue.
I'm thinking that throwing an error when *pending* trigger is dropped
makes sense. Does everyone agree with it ?

> * Not the fault of this patch exactly, but trigger.c seems to have an annoyingly
> large number of copies of the code to look up a trigger by name.  I wonder if we
> could refactor that, say by extracting the guts of
> get_trigger_oid() into an internal function that's passed an already-open
> pg_trigger relation.
While waiting for other reviews and comments, I'm willing to give it a try.

> * Upthread you were complaining about ShareRowExclusiveLock not being a
> strong enough lock, but I think that's nonsense, for the same reason that it's a
> sufficient lock for plain CREATE TRIGGER: if we have that lock then no other
> session can have pending trigger events of any sort on the relation, nor can new
> ones get made before we commit.  But there's no reason to lock out SELECTs on
> the relation, since those don't interact with triggers.
Probably I misunderstand the function's priority like execution of pg_dump
and SELECTs. I'll sort out the information about this.

Other commends and reviews are welcome.

Best,
    Takamichi Osumi

osumi.takamichi@fujitsu.com:
>> * I'm a little bit concerned about the semantics of changing the
>> tgdeferrable/tginitdeferred properties of an existing trigger.  If there are trigger
>> events pending, and the trigger is redefined in such a way that those events
>> should already have been fired, what then?
> OK. I need a discussion about this point.
> There would be two ideas to define the behavior of this semantics change, I think.
> The first idea is to throw an error that means
> the *pending* trigger can't be replaced during the session.
> The second one is just to replace the trigger and ignite the new trigger
> at the end of the session when its tginitdeferred is set true.
> For me, the first one sounds safer. Yet, I'd like to know other opinions.

IMHO, constraint triggers should behave the same in that regard as other 
constraints. I just checked:

BEGIN;
CREATE TABLE t1 (a int CONSTRAINT u UNIQUE DEFERRABLE INITIALLY DEFERRED);
INSERT INTO t1 VALUES (1),(1);
ALTER TABLE t1 ALTER CONSTRAINT u NOT DEFERRABLE;

will throw with:

ERROR:  cannot ALTER TABLE "t1" because it has pending trigger events
SQL state: 55006

So if a trigger event is pending, CREATE OR REPLACE for that trigger 
should throw. I think it should do in any case, not just when changing 
deferrability. This makes it easier to reason about.

If the user has a pending trigger, they can still do SET CONSTRAINTS 
trigger_name IMMEDIATE; to resolve that and then do CREATE OR REPLACE 
TRIGGER, just like in the ALTER TABLE case.

>> regression=# create constraint trigger my_trig after insert on trig_table
>> deferrable initially deferred for each row execute procedure
>> before_replacement(); CREATE TRIGGER regression=# begin; BEGIN
>> regression=*# insert into trig_table default values; INSERT 0 1 regression=*#
>> drop trigger my_trig on trig_table; DROP TRIGGER regression=*# commit;
>> ERROR:  relation 38489 has no triggers
> I could reproduce this bug, using the current master without my patch.
> So this is another issue.
> I'm thinking that throwing an error when *pending* trigger is dropped
> makes sense. Does everyone agree with it ?

Just tested the same example as above, but with DROP TABLE t1; instead 
of ALTER TABLE. This throws with:

ERROR:  cannot DROP TABLE "t1" because it has pending trigger events
SQL state: 55006

So yes, your suggestion makes a lot of sense!

On Thu, Aug 20, 2020 at 12:11 PM osumi.takamichi@fujitsu.com
<osumi.takamichi@fujitsu.com> wrote:
> I have fixed my patch again.

Hi Osumi-san,

FYI - The latest patch (v06) has conflicts when applied.

Kind Regards,
Peter Smith.
Fujitsu Australia

Hi,

Thanks, Peter.
> FYI - The latest patch (v06) has conflicts when applied.

I've fixed my v06 and created v07.
Also, I added one test to throw an error
to avoid a situation that trigger which has pending events are replaced by
any other trigger in the same session.

Best,
    Takamichi Osumi

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

Hi Osumi-san.

I have reviewed the source code of the v07 patch.

(I also reviewed the test cases but I will share those comments as a
separate post).

Below are my comments - sorry, many of them are very minor.

====

COMMENT pg_constraint.c (wrong comment?)

- * The new constraint's OID is returned.
+ * The new constraint's OID is returned. (This will be the same as
+ * "conOid" if that is specified as nonzero.)

Shouldn't that comment say:
(This will be the same as "existing_constraint_oid" if that is other
than InvalidOid)

====

COMMENT pg_constraint.c (declarations)

@@ -91,6 +93,11 @@ CreateConstraintEntry(const char *constraintName,
  NameData cname;
  int i;
  ObjectAddress conobject;
+ SysScanDesc   conscan;
+ ScanKeyData   skey[2];
+ HeapTuple     tuple;
+ bool          replaces[Natts_pg_constraint];
+ Form_pg_constraint constrForm;

Maybe it is more convenient/readable to declare these in the scope
where they are actually used.

====

COMMENT pg_constraint.c (oid checking)

- conOid = GetNewOidWithIndex(conDesc, ConstraintOidIndexId,
- Anum_pg_constraint_oid);
- values[Anum_pg_constraint_oid - 1] = ObjectIdGetDatum(conOid);
+ if (!existing_constraint_oid)
+ {
+ conOid = GetNewOidWithIndex(conDesc, ConstraintOidIndexId,
+ Anum_pg_constraint_oid);

Maybe better to use if (!OidIsValid(existing_constraint_oid)) here.

====

COMMENT tablecmds.c (unrelated change)

-   false); /* is_internal */
+   false); /* is_internal */

Some whitespace which has nothing to do with the patch was changed.

====

COMMENT trigger.c (declarations / whitespace)

+ bool is_update = false;
+ HeapTuple newtup;
+ TupleDesc tupDesc;
+ bool replaces[Natts_pg_trigger];
+ Oid existing_constraint_oid = InvalidOid;
+ bool trigger_exists = false;
+ bool trigger_deferrable = false;

1. One of those variables is misaligned with tabbing.

2. Maybe it is more convenient/readable to declare some of these in
the scope where they are actually used.
e.g. newtup, tupDesc, replaces.

====

COMMENT trigger.c (error messages)

+ /*
+ * If this trigger has pending event, throw an error.
+ */
+ if (stmt->replace && !isInternal && trigger_deferrable &&
+ AfterTriggerPendingOnRel(RelationGetRelid(rel)))
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_IN_USE),
+ errmsg("cannot replace \"%s\" on \"%s\" because it has pending
trigger events",
+ stmt->trigname, RelationGetRelationName(rel))));
+ /*
+ * without OR REPLACE clause, can't override the trigger with the same name.
+ */
+ if (!stmt->replace && !isInternal)
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("trigger \"%s\" for relation \"%s\" already exists",
+ stmt->trigname, RelationGetRelationName(rel))));
+ /*
+ * CREATE OR REPLACE CONSTRAINT TRIGGER command can't replace
non-constraint trigger.
+ */
+ if (stmt->replace && stmt->isconstraint &&
!OidIsValid(existing_constraint_oid))
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("Trigger \"%s\" for relation \"%s\" cannot be replaced with
constraint trigger",
+ stmt->trigname, RelationGetRelationName(rel))));
+ /*
+ * CREATE OR REPLACE TRIGGER command can't replace constraint trigger.
+ */
+ if (stmt->replace && !stmt->isconstraint &&
OidIsValid(existing_constraint_oid))
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("Constraint trigger \"%s\" for relation \"%s\" cannot be
replaced with non-constraint trigger",
+ stmt->trigname, RelationGetRelationName(rel))));


1. The order of these new errors is confusing. Maybe do the "already
exists" check first so that all the REPLACE errors can be grouped
together.

2. There is inconsistent message text capitalising the 1st word.
Should all be lower [1]
[1] - https://www.postgresql.org/docs/current/error-style-guide.html

3. That "already exists" error might benefit from a message hint. e.g.
---
ereport(ERROR, (errcode(ERRCODE_DUPLICATE_OBJECT),
errmsg("trigger \"%s\" for relation \"%s\" already exists", ...),
errhint("use CREATE OR REPLACE to replace it")));
---

4. Those last two errors seem like a complicated way just to say the
trigger types are not compatible. Maybe these messages can be
simplified, and some message hints added. e.g.
---
ereport(ERROR, (errcode(ERRCODE_DUPLICATE_OBJECT),
    errmsg("trigger \"%s\" for relation \"%s\" is a regular trigger", ...),
    errhint("use CREATE OR REPLACE TRIGGER to replace a regular trigger")));


ereport(ERROR, (errcode(ERRCODE_DUPLICATE_OBJECT),
    errmsg("trigger \"%s\" for relation \"%s\" is a constraint trigger", ...),
    errhint("use CREATE OR REPLACE CONSTRAINT to replace a constraint
trigger")));
---

====

COMMENT trigger.c (comment wording)

+ * In case of replace trigger, trigger should no-more dependent on old
+ * referenced objects. Always remove the old dependencies and then

Needs re-wording.

====

Kind Regards,
Peter Smith.
Fujitsu Australia

On Mon, Aug 24, 2020 at 9:33 PM osumi.takamichi@fujitsu.com
<osumi.takamichi@fujitsu.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

Hi, Peter


You gave me two posts for my patch review.
Thank you so much. I'll write all my replies into this post.

> ====
> 
> COMMENT pg_constraint.c (wrong comment?)
> 
> - * The new constraint's OID is returned.
> + * The new constraint's OID is returned. (This will be the same as
> + * "conOid" if that is specified as nonzero.)
> 
> Shouldn't that comment say:
> (This will be the same as "existing_constraint_oid" if that is other than
> InvalidOid)
Thanks. I corrected this part.

> 
> ====
> 
> COMMENT pg_constraint.c (declarations)
> 
> @@ -91,6 +93,11 @@ CreateConstraintEntry(const char *constraintName,
>   NameData cname;
>   int i;
>   ObjectAddress conobject;
> + SysScanDesc   conscan;
> + ScanKeyData   skey[2];
> + HeapTuple     tuple;
> + bool          replaces[Natts_pg_constraint];
> + Form_pg_constraint constrForm;
> 
> Maybe it is more convenient/readable to declare these in the scope where they
> are actually used.
You're right. Fixed.


> ====
> 
> COMMENT pg_constraint.c (oid checking)
> 
> - conOid = GetNewOidWithIndex(conDesc, ConstraintOidIndexId,
> - Anum_pg_constraint_oid);
> - values[Anum_pg_constraint_oid - 1] = ObjectIdGetDatum(conOid);
> + if (!existing_constraint_oid)
> + {
> + conOid = GetNewOidWithIndex(conDesc, ConstraintOidIndexId,
> + Anum_pg_constraint_oid);
> 
> Maybe better to use if (!OidIsValid(existing_constraint_oid)) here.
Got it. I replaced that part by OidIsValid ().


> ====
> 
> COMMENT tablecmds.c (unrelated change)
> 
> -   false); /* is_internal */
> +   false); /* is_internal */
> 
> Some whitespace which has nothing to do with the patch was changed.
Yeah. Fixed.

> ====
> 
> COMMENT trigger.c (declarations / whitespace)
> 
> + bool is_update = false;
> + HeapTuple newtup;
> + TupleDesc tupDesc;
> + bool replaces[Natts_pg_trigger];
> + Oid existing_constraint_oid = InvalidOid; bool trigger_exists = false;
> + bool trigger_deferrable = false;
> 
> 1. One of those variables is misaligned with tabbing.
Fixed.

> 
> 2. Maybe it is more convenient/readable to declare some of these in the scope
> where they are actually used.
> e.g. newtup, tupDesc, replaces.
I cannot do this because those variables are used
at the top level in this function. Anyway, thanks for the comment.

> ====
> 
> COMMENT trigger.c (error messages)
> 
> + /*
> + * If this trigger has pending event, throw an error.
> + */
> + if (stmt->replace && !isInternal && trigger_deferrable &&
> + AfterTriggerPendingOnRel(RelationGetRelid(rel)))
> + ereport(ERROR,
> + (errcode(ERRCODE_OBJECT_IN_USE),
> + errmsg("cannot replace \"%s\" on \"%s\" because it has pending
> trigger events",
> + stmt->trigname, RelationGetRelationName(rel))));
> + /*
> + * without OR REPLACE clause, can't override the trigger with the same name.
> + */
> + if (!stmt->replace && !isInternal)
> + ereport(ERROR,
> + (errcode(ERRCODE_DUPLICATE_OBJECT),
> + errmsg("trigger \"%s\" for relation \"%s\" already exists",
> + stmt->trigname, RelationGetRelationName(rel))));
> + /*
> + * CREATE OR REPLACE CONSTRAINT TRIGGER command can't replace
> non-constraint trigger.
> + */
> + if (stmt->replace && stmt->isconstraint &&
> !OidIsValid(existing_constraint_oid))
> + ereport(ERROR,
> + (errcode(ERRCODE_DUPLICATE_OBJECT),
> + errmsg("Trigger \"%s\" for relation \"%s\" cannot be replaced with
> constraint trigger",
> + stmt->trigname, RelationGetRelationName(rel))));
> + /*
> + * CREATE OR REPLACE TRIGGER command can't replace constraint trigger.
> + */
> + if (stmt->replace && !stmt->isconstraint &&
> OidIsValid(existing_constraint_oid))
> + ereport(ERROR,
> + (errcode(ERRCODE_DUPLICATE_OBJECT),
> + errmsg("Constraint trigger \"%s\" for relation \"%s\" cannot be
> replaced with non-constraint trigger",
> + stmt->trigname, RelationGetRelationName(rel))));
> 
> 
> 1. The order of these new errors is confusing. Maybe do the "already exists"
> check first so that all the REPLACE errors can be grouped together.
OK. I sorted out the order and conditions for this part.

> 2. There is inconsistent message text capitalising the 1st word.
> Should all be lower [1]
> [1] - https://www.postgresql.org/docs/current/error-style-guide.html
Fixed.


> 3. That "already exists" error might benefit from a message hint. e.g.
> ---
> ereport(ERROR, (errcode(ERRCODE_DUPLICATE_OBJECT),
> errmsg("trigger \"%s\" for relation \"%s\" already exists", ...), errhint("use
> CREATE OR REPLACE to replace it")));
> ---
> 
> 4. Those last two errors seem like a complicated way just to say the trigger
> types are not compatible. Maybe these messages can be simplified, and some
> message hints added. e.g.
> ---
> ereport(ERROR, (errcode(ERRCODE_DUPLICATE_OBJECT),
>     errmsg("trigger \"%s\" for relation \"%s\" is a regular trigger", ...),
>     errhint("use CREATE OR REPLACE TRIGGER to replace a regular
> trigger")));
> 
> 
> ereport(ERROR, (errcode(ERRCODE_DUPLICATE_OBJECT),
>     errmsg("trigger \"%s\" for relation \"%s\" is a constraint trigger", ...),
>     errhint("use CREATE OR REPLACE CONSTRAINT to replace a constraint
> trigger")));
I simplified those messages.

> ====
> 
> COMMENT trigger.c (comment wording)
> 
> + * In case of replace trigger, trigger should no-more dependent on old
> + * referenced objects. Always remove the old dependencies and then
> 
> Needs re-wording.
Fixed.


> ====
> 
> 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;
> ---
Got it. I simplified such kind of confusing names. Thanks.


> ====
> 
> 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.
This wasn't necessary. I deleted those.

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

> ====
> 
> 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?
Actually, yes. I wanted readers of this code to easily identify which partitioned is used.
But, I fixed those because it was redundant and not smart. Your suggestion sounds better.

> 
> 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
This makes sense. I adopted your idea.

> ====
> 
> 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.
I've added those tests additionally.

> ====
> 
> 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.
I modified the names of functions.


Best,
    Takamichi Osumi

Hi Osumi-san.

Thanks for addressing my previous review comments in your new v08 patch.

I have checked it again.

My only remaining comments are for trivial stuff:

====

COMMENT trigger.c (tab alignment)

@@ -184,6 +185,13 @@ CreateTrigger(CreateTrigStmt *stmt, const char
*queryString,
  char    *oldtablename = NULL;
  char    *newtablename = NULL;
  bool partition_recurse;
+ bool is_update = false;
+ HeapTuple newtup;
+ TupleDesc tupDesc;
+ bool replaces[Natts_pg_trigger];
+ Oid existing_constraint_oid = InvalidOid;
+ bool trigger_exists = false;
+ bool trigger_deferrable = false;

Maybe my editor configuration is wrong, but the alignment of
"existing_constraint_oid" still does not look fixed to me.

====

COMMENT trigger.c (move some declarations)

>> 2. Maybe it is more convenient/readable to declare some of these in the scope
>> where they are actually used.
>> e.g. newtup, tupDesc, replaces.
>I cannot do this because those variables are used
>at the top level in this function. Anyway, thanks for the comment.

Are you sure it can't be done? It looks doable to me.

====

COMMENT trigger.c ("already exists" message)

In my v07 review I suggested adding a hint for the new "already exists" error.
Of course choice is yours, but since you did not add it I just wanted
to ask was that accidental or deliberate?

====

COMMENT triggers.sql/.out (typo in comment)

+-- test for detecting imcompatible replacement of trigger

"imcompatible" -> "incompatible"

====

COMMENT triggers.sql/.out (wrong comment)

+drop trigger my_trig on my_table;
+create or replace constraint trigger my_trig
+  after insert on my_table
+  for each row execute procedure funcB(); --should fail
+create or replace trigger my_trig
+  after insert on my_table
+  for each row execute procedure funcB(); --should fail
+ERROR:  trigger "my_trig" for relation "my_table" is a constraint trigger
+HINT:  use CREATE OR REPLACE CONSTRAINT TRIGGER to replace a costraint trigger

I think the first "--should fail" comment is misleading. First time is OK.

====

Kind Regards,
Peter Smith.
Fujitsu Australia

Hi, Peter-San

I've fixed all except one point.
> My only remaining comments are for trivial stuff:
Not trivial but important.

> COMMENT trigger.c (tab alignment)
> 
> @@ -184,6 +185,13 @@ CreateTrigger(CreateTrigStmt *stmt, const char
> *queryString,
>   char    *oldtablename = NULL;
>   char    *newtablename = NULL;
>   bool partition_recurse;
> + bool is_update = false;
> + HeapTuple newtup;
> + TupleDesc tupDesc;
> + bool replaces[Natts_pg_trigger];
> + Oid existing_constraint_oid = InvalidOid; bool trigger_exists = false;
> + bool trigger_deferrable = false;
> 
> Maybe my editor configuration is wrong, but the alignment of
> "existing_constraint_oid" still does not look fixed to me.
You were right. In order to solve this point completely,
I've executed pgindent and gotten clean alignment.
How about v09 ?
Other alignments of C source codes have been fixed as well.
This is mainly comments, though.

> COMMENT trigger.c (move some declarations)
> 
> >> 2. Maybe it is more convenient/readable to declare some of these in
> >> the scope where they are actually used.
> >> e.g. newtup, tupDesc, replaces.
> >I cannot do this because those variables are used at the top level in
> >this function. Anyway, thanks for the comment.
> 
> Are you sure it can't be done? It looks doable to me.
Done. I was wrong. Thank you.

> COMMENT trigger.c ("already exists" message)
> 
> In my v07 review I suggested adding a hint for the new "already exists" error.
> Of course choice is yours, but since you did not add it I just wanted to ask was
> that accidental or deliberate?
This was deliberate.
The code path of "already exists" error you mentioned above
is used for other errors as well. For example, a failure case of 
"ALTER TABLE name ATTACH PARTITION partition_name...".

This command fails if the "partition_name" table has a trigger,
whose name is exactly same as the trigger on the "name" table.
For each user-defined row-level trigger that exists in the "name" table,
a corresponding one is created in the attached table, automatically.
Thus, the "ALTER TABLE" command throws the error which says
trigger "name" for relation "partition_name" already exists.
I felt if I add the hint, application developer would get confused.
Did it make sense ?

> COMMENT triggers.sql/.out (typo in comment)
> 
> +-- test for detecting imcompatible replacement of trigger
> 
> "imcompatible" -> "incompatible"
Fixed.


> COMMENT triggers.sql/.out (wrong comment)
> 
> +drop trigger my_trig on my_table;
> +create or replace constraint trigger my_trig
> +  after insert on my_table
> +  for each row execute procedure funcB(); --should fail create or
> +replace trigger my_trig
> +  after insert on my_table
> +  for each row execute procedure funcB(); --should fail
> +ERROR:  trigger "my_trig" for relation "my_table" is a constraint
> +trigger
> +HINT:  use CREATE OR REPLACE CONSTRAINT TRIGGER to replace a
> costraint
> +trigger
> 
> I think the first "--should fail" comment is misleading. First time is OK.
Thanks. Removed the misleading comment.


Regards,
    Takamichi Osumi

On Tue, Sep 8, 2020 at 9:36 PM osumi.takamichi@fujitsu.com
<osumi.takamichi@fujitsu.com> wrote:
>
> I've fixed all except one point.

Thanks for addressing my previous review comments in your new v09 patch.

Those are fixed OK now, but I found 2 new review points.

====

COMMENT trigger.c (typo)

+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("trigger \"%s\" for relation \"%s\" is a constraint trigger",
+ stmt->trigname, RelationGetRelationName(rel)),
+ errhint("use CREATE OR REPLACE CONSTRAINT TRIGGER to replace a
costraint trigger")));


Typo in the errhint text.
"costraint" -> "constraint"

====

COMMENT create_trigger.sgmg (add more help?)

I noticed that the CREATE OR REPLACE FUNCTION help [1] describes the
OR REPLACE syntax ("Description" section) and also mentions some of
the restrictions when using REPLACE ("Notes" section).
[1] - https://www.postgresql.org/docs/current/sql-createfunction.html

~~
OTOH this trigger patch does not add anything much at all in the trigger help.

Shouldn't the "Description" at least say something like:
"CREATE OR REPLACE will either create a new trigger, or replace an
existing definition."

Shouldn't the "Notes" include information about restrictions when
using OR REPLACE
e.g. cannot replace triggers with triggers of a different kind
e.g. cannot replace triggers with pending events

What do you think?

====

Kind Regards,
Peter Smith.
Fujitsu Australia

Hi


> Those are fixed OK now, but I found 2 new review points.
> 
> ====
> 
> COMMENT trigger.c (typo)
> 
> + ereport(ERROR,
> + (errcode(ERRCODE_DUPLICATE_OBJECT),
> + errmsg("trigger \"%s\" for relation \"%s\" is a constraint trigger",
> + stmt->trigname, RelationGetRelationName(rel)),
> + errhint("use CREATE OR REPLACE CONSTRAINT TRIGGER to replace a
> costraint trigger")));
> 
> 
> Typo in the errhint text.
> "costraint" -> "constraint"
Fixed. Thank you.

> ====
> 
> COMMENT create_trigger.sgmg (add more help?)
> 
> I noticed that the CREATE OR REPLACE FUNCTION help [1] describes the OR
> REPLACE syntax ("Description" section) and also mentions some of the
> restrictions when using REPLACE ("Notes" section).
> [1] - https://www.postgresql.org/docs/current/sql-createfunction.html
> 
> ~~
> OTOH this trigger patch does not add anything much at all in the trigger help.
> 
> Shouldn't the "Description" at least say something like:
> "CREATE OR REPLACE will either create a new trigger, or replace an existing
> definition."
> 
> Shouldn't the "Notes" include information about restrictions when using OR
> REPLACE e.g. cannot replace triggers with triggers of a different kind e.g.
> cannot replace triggers with pending events
> 
> What do you think?
That's a great idea. I've applied this idea to the latest patch v10.

Regards,
    Takamichi Osumi

On Wed, Sep 9, 2020 at 11:28 PM osumi.takamichi@fujitsu.com
<osumi.takamichi@fujitsu.com> wrote:
> That's a great idea. I've applied this idea to the latest patch v10.

====

COMMENT create_trigger.sgml (typo/wording)

"vise versa" -> "vice versa"

BEFORE
You cannot replace triggers with a different type of trigger, that
means it is impossible to replace regular trigger with constraint
trigger and vise versa.

AFTER (suggestion)
You cannot replace triggers with a different type of trigger. That
means it is impossible to replace a regular trigger with a constraint
trigger, and vice versa.

====

Kind Regards,
Peter Smith.
Fujitsu Australia

Hello, Peter-San


> > That's a great idea. I've applied this idea to the latest patch v10.
> 
> ====
> 
> COMMENT create_trigger.sgml (typo/wording)
> 
> "vise versa" -> "vice versa"
Sorry and thank you for all your pointing out.

> BEFORE
> You cannot replace triggers with a different type of trigger, that means it is
> impossible to replace regular trigger with constraint trigger and vise versa.
> 
> AFTER (suggestion)
> You cannot replace triggers with a different type of trigger. That means it is
> impossible to replace a regular trigger with a constraint trigger, and vice versa.
Thank you. Your suggestion must be better.

I attached the v11 patch.

Regards,
    Takamichi Osumi

On Thu, Sep 10, 2020 at 12:34 PM osumi.takamichi@fujitsu.com
<osumi.takamichi@fujitsu.com> wrote:
> I attached the v11 patch.

The v11 patch looked OK to me.

Since I have no more review comments I am marking this as "ready for committer".

Kind Regards,
Peter Smith.
Fujitsu Australia

"osumi.takamichi@fujitsu.com" <osumi.takamichi@fujitsu.com> writes:
> [ CREATE_OR_REPLACE_TRIGGER_v11.patch ]

I took a quick look through this.  I think there's still work left to do.

* I'm concerned by the fact that there doesn't seem to be any defense
against somebody replacing a foreign-key trigger with something that
does something else entirely, and thereby silently breaking their
foreign key constraint.  I think it might be a good idea to forbid
replacing triggers for which tgisinternal is true; but I've not done
the legwork to see if that's exactly the condition we want.

* In the same vein, I'm not sure that the right things happen when fooling
with triggers attached to partitioned tables.  We presumably don't want to
allow mucking directly with a child trigger.  Perhaps refusing an update
when tgisinternal might fix this too (although we'll have to be careful to
make the error message not too confusing).

* I don't think that you've fully thought through the implications
of replacing a trigger for a table that the current transaction has
already modified.  Is it really sufficient, or even useful, to do
this:

+            /*
+             * If this trigger has pending events, throw an error.
+             */
+            if (trigger_deferrable && AfterTriggerPendingOnRel(RelationGetRelid(rel)))

As an example, if we change a BEFORE trigger to an AFTER trigger,
that's not going to affect the fact that we *already* fired that
trigger.  Maybe this is okay and we just need to document it, but
I'm not convinced.

* BTW, I don't think a trigger necessarily has to be deferrable
in order to have pending AFTER events.  The existing use of
AfterTriggerPendingOnRel certainly doesn't assume that.  But really,
I think we probably ought to be applying CheckTableNotInUse which'd
include that test.  (Another way in which there's fuzzy thinking
here is that AfterTriggerPendingOnRel isn't specific to *this*
trigger.)

* A lesser point is that I think you're overcomplicating the
code by applying heap_modify_tuple.  You might as well just
build the new tuple normally in all cases, and then apply
either CatalogTupleInsert or CatalogTupleUpdate.

* Also, the search for an existing trigger tuple is being
done the hard way.  You're using an index on (tgrelid, tgname),
so you could include the name in the index key and expect that
there's at most one match.

            regards, tom lane

Hi


I spent too much time to respond to this e-mail. Sorry.
Actually, I got stuck to deal with achieving both
error detection of internal trigger case and pending trigger case.

> * I'm concerned by the fact that there doesn't seem to be any defense against
> somebody replacing a foreign-key trigger with something that does something
> else entirely, and thereby silently breaking their foreign key constraint.  I think
> it might be a good idea to forbid replacing triggers for which tgisinternal is true;
> but I've not done the legwork to see if that's exactly the condition we want.
>
> * In the same vein, I'm not sure that the right things happen when fooling with
> triggers attached to partitioned tables.  We presumably don't want to allow
> mucking directly with a child trigger.  Perhaps refusing an update when
> tgisinternal might fix this too (although we'll have to be careful to make the error
> message not too confusing).
Yeah, you are right. tgisinternal works to detect an invalid cases.
I added a new check condition in my patch to prohibit
the replacement of internal triggers by an user,
which protects FK trigger and child trigger from being replaced directly.


> * I don't think that you've fully thought through the implications of replacing a
> trigger for a table that the current transaction has already modified.  Is it really
> sufficient, or even useful, to do
> this:
>
> +            /*
> +             * If this trigger has pending events, throw an error.
> +             */
> +            if (trigger_deferrable &&
> + AfterTriggerPendingOnRel(RelationGetRelid(rel)))
>
> As an example, if we change a BEFORE trigger to an AFTER trigger, that's not
> going to affect the fact that we *already* fired that trigger.  Maybe this is okay
> and we just need to document it, but I'm not convinced.
>
> * BTW, I don't think a trigger necessarily has to be deferrable in order to have
> pending AFTER events.  The existing use of AfterTriggerPendingOnRel
> certainly doesn't assume that.  But really, I think we probably ought to be
> applying CheckTableNotInUse which'd include that test.  (Another way in
> which there's fuzzy thinking here is that AfterTriggerPendingOnRel isn't specific
> to *this*
> trigger.)
Hmm, actually, when I just put a code of CheckTableNotInUse() in CreateTrigger(),
it throws error like "cannot CREATE OR REPLACE
TRIGGER because it is being used by active queries in this session".
This causes an break of the protection for internal cases above
and a contradiction of already passed test cases.
I though adding a condition of in_partition==false to call
CheckTableNotInUse(). But this doesn't work in a corner case that
child trigger generated internally is pending and
we don't want to allow the replacement of this kind of trigger.
Did you have any good idea to achieve both points at the same time ?


> * A lesser point is that I think you're overcomplicating the code by applying
> heap_modify_tuple.  You might as well just build the new tuple normally in all
> cases, and then apply either CatalogTupleInsert or CatalogTupleUpdate.
>
> * Also, the search for an existing trigger tuple is being done the hard way.
> You're using an index on (tgrelid, tgname), so you could include the name in the
> index key and expect that there's at most one match.
While waiting for a new reply, I'll doing those 2 refactorings.


Regards,
    Takamichi Osumi

Hello


> > * A lesser point is that I think you're overcomplicating the code by
> > applying heap_modify_tuple.  You might as well just build the new
> > tuple normally in all cases, and then apply either CatalogTupleInsert or
> CatalogTupleUpdate.
> >
> > * Also, the search for an existing trigger tuple is being done the hard way.
> > You're using an index on (tgrelid, tgname), so you could include the
> > name in the index key and expect that there's at most one match.
> While waiting for a new reply, I'll doing those 2 refactorings.
I'm done with those refactorings. Please have a look at the changes
of the latest patch.

> > * I don't think that you've fully thought through the implications of
> > replacing a trigger for a table that the current transaction has
> > already modified.  Is it really sufficient, or even useful, to do
> > this:
> >
> > +            /*
> > +             * If this trigger has pending events, throw an error.
> > +             */
> > +            if (trigger_deferrable &&
> > + AfterTriggerPendingOnRel(RelationGetRelid(rel)))
> >
> > As an example, if we change a BEFORE trigger to an AFTER trigger,
> > that's not going to affect the fact that we *already* fired that
> > trigger.  Maybe this is okay and we just need to document it, but I'm not
> convinced.
> >
> > * BTW, I don't think a trigger necessarily has to be deferrable in
> > order to have pending AFTER events.  The existing use of
> > AfterTriggerPendingOnRel certainly doesn't assume that.  But really, I
> > think we probably ought to be applying CheckTableNotInUse which'd
> > include that test.  (Another way in which there's fuzzy thinking here
> > is that AfterTriggerPendingOnRel isn't specific to *this*
> > trigger.)
> Hmm, actually, when I just put a code of CheckTableNotInUse() in
> CreateTrigger(), it throws error like "cannot CREATE OR REPLACE TRIGGER
> because it is being used by active queries in this session".
> This causes an break of the protection for internal cases above and a
> contradiction of already passed test cases.
> I though adding a condition of in_partition==false to call CheckTableNotInUse().
> But this doesn't work in a corner case that child trigger generated internally is
> pending and we don't want to allow the replacement of this kind of trigger.
> Did you have any good idea to achieve both points at the same time ?
Still, in terms of this point, I'm waiting for a comment !


Regards,
    Takamichi Osumi

From: osumi.takamichi@fujitsu.com <osumi.takamichi@fujitsu.com>
> > > * I don't think that you've fully thought through the implications
> > > of replacing a trigger for a table that the current transaction has
> > > already modified.  Is it really sufficient, or even useful, to do
> > > this:
> > >
> > > +            /*
> > > +             * If this trigger has pending events, throw an error.
> > > +             */
> > > +            if (trigger_deferrable &&
> > > + AfterTriggerPendingOnRel(RelationGetRelid(rel)))
> > >
> > > As an example, if we change a BEFORE trigger to an AFTER trigger,
> > > that's not going to affect the fact that we *already* fired that
> > > trigger.  Maybe this is okay and we just need to document it, but
> > > I'm not
> > convinced.
> > >
> > > * BTW, I don't think a trigger necessarily has to be deferrable in
> > > order to have pending AFTER events.  The existing use of
> > > AfterTriggerPendingOnRel certainly doesn't assume that.  But really,
> > > I think we probably ought to be applying CheckTableNotInUse which'd
> > > include that test.  (Another way in which there's fuzzy thinking
> > > here is that AfterTriggerPendingOnRel isn't specific to *this*
> > > trigger.)
> > Hmm, actually, when I just put a code of CheckTableNotInUse() in
> > CreateTrigger(), it throws error like "cannot CREATE OR REPLACE
> > TRIGGER because it is being used by active queries in this session".
> > This causes an break of the protection for internal cases above and a
> > contradiction of already passed test cases.
> > I though adding a condition of in_partition==false to call
> CheckTableNotInUse().
> > But this doesn't work in a corner case that child trigger generated
> > internally is pending and we don't want to allow the replacement of this kind
> of trigger.
> > Did you have any good idea to achieve both points at the same time ?
> Still, in terms of this point, I'm waiting for a comment !

I understand this patch is intended for helping users to migrate from other DBMSs (mainly Oracle?) because they can
easilyalter some trigger attributes (probably the trigger action and WHEN condition in practice.)  OTOH, the above
issueseems to be associated with the Postgres-specific constraint trigger that is created with CONSTRAINT clause.
(Oracleand the SQL standard doesn't have an equivalent feature.) 

So, how about just disallowing the combination of REPLACE and CONSTRAINT?  I think nobody would be crippled with that.
Ifsomeone wants the combination by all means, that can be a separate enhancement. 


Regards
Takayuki Tsunakawa

Hi,


From: Tsunakawa, Takayuki < tsunakawa.takay@fujitsu.com>
> From: osumi.takamichi@fujitsu.com <osumi.takamichi@fujitsu.com>
> > > > * I don't think that you've fully thought through the implications
> > > > of replacing a trigger for a table that the current transaction
> > > > has already modified.  Is it really sufficient, or even useful, to
> > > > do
> > > > this:
> > > >
> > > > +            /*
> > > > +             * If this trigger has pending events, throw an error.
> > > > +             */
> > > > +            if (trigger_deferrable &&
> > > > + AfterTriggerPendingOnRel(RelationGetRelid(rel)))
> > > >
> > > > As an example, if we change a BEFORE trigger to an AFTER trigger,
> > > > that's not going to affect the fact that we *already* fired that
> > > > trigger.  Maybe this is okay and we just need to document it, but
> > > > I'm not
> > > convinced.
> > > >
> > > > * BTW, I don't think a trigger necessarily has to be deferrable in
> > > > order to have pending AFTER events.  The existing use of
> > > > AfterTriggerPendingOnRel certainly doesn't assume that.  But
> > > > really, I think we probably ought to be applying
> > > > CheckTableNotInUse which'd include that test.  (Another way in
> > > > which there's fuzzy thinking here is that AfterTriggerPendingOnRel
> > > > isn't specific to *this*
> > > > trigger.)
> > > Hmm, actually, when I just put a code of CheckTableNotInUse() in
> > > CreateTrigger(), it throws error like "cannot CREATE OR REPLACE
> > > TRIGGER because it is being used by active queries in this session".
> > > This causes an break of the protection for internal cases above and
> > > a contradiction of already passed test cases.
> > > I though adding a condition of in_partition==false to call
> > CheckTableNotInUse().
> > > But this doesn't work in a corner case that child trigger generated
> > > internally is pending and we don't want to allow the replacement of
> > > this kind
> > of trigger.
> > > Did you have any good idea to achieve both points at the same time ?
> > Still, in terms of this point, I'm waiting for a comment !
>
> I understand this patch is intended for helping users to migrate from other
> DBMSs (mainly Oracle?) because they can easily alter some trigger attributes
> (probably the trigger action and WHEN condition in practice.)  OTOH, the
> above issue seems to be associated with the Postgres-specific constraint
> trigger that is created with CONSTRAINT clause.  (Oracle and the SQL
> standard doesn't have an equivalent feature.)
>
> So, how about just disallowing the combination of REPLACE and
> CONSTRAINT?  I think nobody would be crippled with that.  If someone
> wants the combination by all means, that can be a separate enhancement.
I didn't notice this kind of perspective and you are right.
In order to achieve the purpose to help database migration from Oracle to Postgres,
prohibitting the usage of OR REPLACE for constraint trigger is no problem.

Thanks for your great advice. I fixed and created new version.
Also, the size of this patch becomes much smaller.


Best,
    Takamichi Osumi

Hello Osumi-san.

Below are my v14 patch review comments for your consideration.

===

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

===

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

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

===

(3) COMMENT
File: src/backend/commands/trigger.c
@@ -185,6 +185,10 @@ CreateTrigger(CreateTrigStmt *stmt, const char
*queryString,
+ bool internal_trigger = false;

--

There is potential for confusion of "isInternal" versus
"internal_trigger". The meaning is not apparent from the names, but
IIUC isInternal seems to be when creating an internal trigger, whereas
internal_trigger seems to be when you found an existing trigger that
was previously created as isInternal.

Maybe something like "existing_isInternal" would be a better name
instead of "internal_trigger".

===

(4) COMMENT
File: src/backend/commands/trigger.c
@@ -185,6 +185,10 @@ CreateTrigger(CreateTrigStmt *stmt, const char
*queryString,
+ bool is_update = false;

Consider if "was_replaced" might be a better name than "is_update".

===

(5) COMMENT
File: src/backend/commands/trigger.c
@@ -669,6 +673,81 @@ CreateTrigger(CreateTrigStmt *stmt, const char
*queryString,
+ if (!stmt->replace)
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("trigger \"%s\" for relation \"%s\" already exists",
+ stmt->trigname, RelationGetRelationName(rel))));
+ else
+ {
+ /*
+ * An internal trigger cannot be replaced by another user defined
+ * trigger. This should exclude the case that internal trigger is
+ * child trigger of partition table and needs to be rewritten when
+ * the parent trigger is replaced by user.
+ */
+ if (internal_trigger && isInternal == false && in_partition == false)
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("trigger \"%s\" for relation \"%s\" is an internal trigger",
+ stmt->trigname, RelationGetRelationName(rel))));
+
+ /*
+ * CREATE OR REPLACE TRIGGER command can't replace constraint
+ * trigger.
+ */
+ if (OidIsValid(existing_constraint_oid))
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("trigger \"%s\" for relation \"%s\" is a constraint trigger",
+ stmt->trigname, RelationGetRelationName(rel))));
+ }

It is not really necessary for the "OR REPLACE" code to need to be
inside an "else" block like that because the "if (!stmt->replace)" has
already been tested above. Consider removing the "else {" to remove
unnecessary indent if you want to.

===

(6) COMMENT
File: src/backend/commands/trigger.c
(same code block as above)

Condition is strangely written:
e.g.
Before: if (internal_trigger && isInternal == false && in_partition == false)
After: if (internal_trigger && !isInternal && !in_partition)

===

(7) COMMENT
File: src/backend/commands/trigger.c
(same code block as above)
/*
 * CREATE OR REPLACE TRIGGER command can't replace constraint
 * trigger.
 */

--

Only need to say
/* Can't replace a constraint trigger. */

===

(8) COMMENT
File: src/include/nodes/parsenodes.h
@@ -2429,6 +2429,9 @@ typedef struct CreateAmStmt

The comment does not need to say "when true,". Just saying "replace
trigger if already exists" is enough.

===

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

===

Kind Regards,
Peter Smith.
Fujitsu Australia

Hi


Peter-San, thanks for your support.
On Monday, November 2, 2020 2:39 PM Peter Smith 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 ?

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

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

> ===
> 
> (3) COMMENT
> File: src/backend/commands/trigger.c
> @@ -185,6 +185,10 @@ CreateTrigger(CreateTrigStmt *stmt, const char
> *queryString,
> + bool internal_trigger = false;
> 
> --
> 
> There is potential for confusion of "isInternal" versus "internal_trigger". The
> meaning is not apparent from the names, but IIUC isInternal seems to be
> when creating an internal trigger, whereas internal_trigger seems to be when
> you found an existing trigger that was previously created as isInternal.
> 
> Maybe something like "existing_isInternal" would be a better name instead of
> "internal_trigger".
Definitely sounds better. I fixed the previous confusing name.

> 
> ===
> 
> (4) COMMENT
> File: src/backend/commands/trigger.c
> @@ -185,6 +185,10 @@ CreateTrigger(CreateTrigStmt *stmt, const char
> *queryString,
> + bool is_update = false;
> 
> Consider if "was_replaced" might be a better name than "is_update".
> 
> ===
Also, this must be good. Done.

> 
> (5) COMMENT
> File: src/backend/commands/trigger.c
> @@ -669,6 +673,81 @@ CreateTrigger(CreateTrigStmt *stmt, const char
> *queryString,
> + if (!stmt->replace)
> + ereport(ERROR,
> + (errcode(ERRCODE_DUPLICATE_OBJECT),
> + errmsg("trigger \"%s\" for relation \"%s\" already exists",
> + stmt->trigname, RelationGetRelationName(rel))));
> + else
> + {
> + /*
> + * An internal trigger cannot be replaced by another user defined
> + * trigger. This should exclude the case that internal trigger is
> + * child trigger of partition table and needs to be rewritten when
> + * the parent trigger is replaced by user.
> + */
> + if (internal_trigger && isInternal == false && in_partition == false)
> + ereport(ERROR, (errcode(ERRCODE_DUPLICATE_OBJECT),
> + errmsg("trigger \"%s\" for relation \"%s\" is an internal trigger",
> + stmt->trigname, RelationGetRelationName(rel))));
> +
> + /*
> + * CREATE OR REPLACE TRIGGER command can't replace constraint
> + * trigger.
> + */
> + if (OidIsValid(existing_constraint_oid))
> + ereport(ERROR,
> + (errcode(ERRCODE_DUPLICATE_OBJECT),
> + errmsg("trigger \"%s\" for relation \"%s\" is a constraint trigger",
> + stmt->trigname, RelationGetRelationName(rel))));
> + }
> 
> It is not really necessary for the "OR REPLACE" code to need to be inside an
> "else" block like that because the "if (!stmt->replace)" has already been
> tested above. Consider removing the "else {" to remove unnecessary indent if
> you want to.
Yeah, you are right. Fixed.

> ===
> 
> (6) COMMENT
> File: src/backend/commands/trigger.c
> (same code block as above)
> 
> Condition is strangely written:
> e.g.
> Before: if (internal_trigger && isInternal == false && in_partition == false)
> After: if (internal_trigger && !isInternal && !in_partition)
OK. Done

> 
> ===
> 
> (7) COMMENT
> File: src/backend/commands/trigger.c
> (same code block as above)
> /*
>  * CREATE OR REPLACE TRIGGER command can't replace constraint
>  * trigger.
>  */
> 
> --
> 
> Only need to say
> /* Can't replace a constraint trigger. */
> 
> ===
> 
> (8) COMMENT
> File: src/include/nodes/parsenodes.h
> @@ -2429,6 +2429,9 @@ typedef struct CreateAmStmt
> 
> The comment does not need to say "when true,". Just saying "replace trigger
> if already exists" is enough.
> 
> ===
Applied.


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


Best,
    Takamichi Osumi

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

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

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

Hello


On Friday, November 6, 2020 2:25 PM Peter Smith <smithpb2250@gmail.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

Hello Osumi-san.

I have checked the latest v17 patch w.r.t. to my previous comments.

The v17 patch applies cleanly.

make check is successful.

The regenerated docs look OK.

I have no further review comments, so have flagged this v17 as "ready
for committer" - https://commitfest.postgresql.org/30/2307/

---

Kind Regards,
Peter Smith.
Fujitsu Australia

On Sat, Nov 7, 2020 at 10:00 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hello Osumi-san.
>
> I have checked the latest v17 patch w.r.t. to my previous comments.
>
> The v17 patch applies cleanly.
>
> make check is successful.
>
> The regenerated docs look OK.
>
> I have no further review comments, so have flagged this v17 as "ready
> for committer" - https://commitfest.postgresql.org/30/2307/
>

The patch looks fine to me however I feel that in the test case there
are a lot of duplicate statement which can be reduced
e.g.
+-- 1. Overwrite existing regular trigger with regular trigger
(without OR REPLACE)
+create trigger my_trig
+  after insert on my_table
+  for each row execute procedure funcA();
+create trigger my_trig
+  after insert on my_table
+  for each row execute procedure funcB(); -- should fail
+drop trigger my_trig on my_table;
+
+-- 2. Overwrite existing regular trigger with regular trigger (with OR REPLACE)
+create trigger my_trig
+  after insert on my_table
+  for each row execute procedure funcA();
+insert into my_table values (1);
+create or replace trigger my_trig
+  after insert on my_table
+  for each row execute procedure funcB(); -- OK
+insert into my_table values (1);
+drop trigger my_trig on my_table;

In this test, test 1 failed because it tried to change the trigger
function without OR REPLACE, which is fine but now test 2 can continue
from there, I mean we don't need to drop the trigger at end of the
test1 and then test2 can try it with OR REPLACE syntax.  This way we
can reduce the extra statement execution which is not necessary.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Hi,


On Saturday, Nov 7, 2020 2:06 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> The patch looks fine to me however I feel that in the test case there are a lot
> of duplicate statement which can be reduced e.g.
> +-- 1. Overwrite existing regular trigger with regular trigger
> (without OR REPLACE)
> +create trigger my_trig
> +  after insert on my_table
> +  for each row execute procedure funcA(); create trigger my_trig
> +  after insert on my_table
> +  for each row execute procedure funcB(); -- should fail drop trigger
> +my_trig on my_table;
> +
> +-- 2. Overwrite existing regular trigger with regular trigger (with OR
> +REPLACE) create trigger my_trig
> +  after insert on my_table
> +  for each row execute procedure funcA(); insert into my_table values
> +(1); create or replace trigger my_trig
> +  after insert on my_table
> +  for each row execute procedure funcB(); -- OK insert into my_table
> +values (1); drop trigger my_trig on my_table;
> 
> In this test, test 1 failed because it tried to change the trigger function without
> OR REPLACE, which is fine but now test 2 can continue from there, I mean we
> don't need to drop the trigger at end of the
> test1 and then test2 can try it with OR REPLACE syntax.  This way we can
> reduce the extra statement execution which is not necessary.
OK. That makes sense.

Attached the revised version.
The tests in this patch should not include redundancy.
I checked the tests of trigger replacement for partition tables as well.

Here, I did not and will not delete the comments with numbering from 1 to 8 so that
other developers can check if the all cases are listed up or not easily.


Best,
    Takamichi Osumi

"osumi.takamichi@fujitsu.com" <osumi.takamichi@fujitsu.com> writes:
> [ CREATE_OR_REPLACE_TRIGGER_v18.patch ]

Pushed with some mostly-minor cleanup.

(I know this has been a very long slog.  Congratulations for
seeing it through.)

            regards, tom lane