Re: extension patch of CREATE OR REPLACE TRIGGER

"osumi(dot)takamichi(at)fujitsu(dot)com" <osumi(dot)takamichi(at)fujitsu(dot)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