Re: extension patch of CREATE OR REPLACE TRIGGER

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