RE: extension patch of CREATE OR REPLACE TRIGGER

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