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

Hi Takamichi Osumi,
On Tue, Jul 9, 2019

I've rebased the previous patch to be applied

I don't test your patch fully yet but here are same comment.
There are same white space issue like here
-  bool is_internal)
+  bool is_internal,
+  Oid existing_constraint_oid)
in a few place

+ // trigoid = HeapTupleGetOid(tuple); // raw code
please remove this line if you don't use it.

+ if(!existing_constraint_oid){
+ conOid = GetNewOidWithIndex(conDesc, ConstraintOidIndexId,
+ Anum_pg_constraint_oid);
+ values[Anum_pg_constraint_oid - 1] = ObjectIdGetDatum(conOid);
+ }
incorrect bracing style here and its appear in a few other places too
and it seems to me that the change in regression test is
huge can you reduce it?

regards
Surafel

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.

Regards,
Takamichi Osumi

I don't test your patch fully yet but here are same comment.
There are same white space issue like here
-  bool is_internal)
+  bool is_internal,
+  Oid existing_constraint_oid)
in a few place

+ // trigoid = HeapTupleGetOid(tuple); // raw code
please remove this line if you don't use it.

+ if(!existing_constraint_oid){
+ conOid = GetNewOidWithIndex(conDesc, ConstraintOidIndexId,
+ Anum_pg_constraint_oid);
+ values[Anum_pg_constraint_oid - 1] = ObjectIdGetDatum(conOid);
+ }
incorrect bracing style here and its appear in a few other places too
and it seems to me that the change in regression test is
huge can you reduce it?

regards
Surafel

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

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