Hi, First things first, thanks for the review!
I'm working on a new version of the patch to fix all the specific comments you called "nitpicking" here and in your previous email. This new patch will also implement a single list of triggers to run in alphabetical order, not split by ANY/specific command. Robert Haas <robertmh...@gmail.com> writes: > I am extremely concerned about the way in which this patch arranges to > invoke command triggers. You've got call sites spattered all over the > place, and I think that's going to be, basically, an unfixable mess > and a breeding ground for bugs. On a first read-through: In the first versions of the patch I did try to have a single point where to integrate the feature and that didn't work out. If you want to publish information such as object id, name and schema you need to have specialized code spread out everywhere. Then about where to call the trigger, it's a per-command decision with a general policy. Your comments here are of two kinds, mostly about having to guess the policy because it's not explicit, and some specifics that either are in question or not following up on the policy. The policy I've been willing to install is that command triggers should get fired once the basic error checking is done. That's not perfect for auditing purposes *if you want to log all attempts* but it's good enough to audit all commands that had an impact on your system, and you still can block commands. Also, in most commands you can't get the object id and name before the basic error checking is done. Now, about the IF NOT EXISTS case, with the policy just described there's no reason to trigger user code, but I can also see your position here. > 1. BEFORE ALTER TABLE triggers are fired in AlterTable(). However, an I used to have that in utility.c but Andres had me move that out, and it seems like I should get back to utility.c for the reasons you're mentioning and that I missed. > 2. BEFORE CREATE TABLE triggers seem to have similar issues; see > transformCreateStmt. > > rhaas=# create table foo (a serial primary key); > NOTICE: CREATE TABLE will create implicit sequence "foo_a_seq" for > serial column "foo.a" > NOTICE: snitch: BEFORE CREATE SEQUENCE public.foo_a_seq [<NULL>] > NOTICE: snitch: AFTER CREATE SEQUENCE public.foo_a_seq [16392] > NOTICE: snitch: BEFORE CREATE TABLE public.foo [<NULL>] > NOTICE: snitch: BEFORE CREATE INDEX public.foo_pkey [<NULL>] > NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index > "foo_pkey" for table "foo" > NOTICE: snitch: AFTER CREATE INDEX public.foo_pkey [16398] > NOTICE: snitch: BEFORE ALTER SEQUENCE public.foo_a_seq [16392] > NOTICE: snitch: AFTER ALTER SEQUENCE public.foo_a_seq [16392] > NOTICE: snitch: AFTER CREATE TABLE public.foo [16394] > CREATE TABLE That's meant to be a feature of the command trigger system, that's been asked about by a lot of people. Having triggers fire for sub commands is possibly The Right Thing™, or at least the most popular one. > 3. RemoveRelations() and RemoveObjects() similarly take the position > that when the object does not exist, command triggers need not fire. > This seems entirely arbitrary. CREATE EXTENSION IF NOT EXISTS, > however, takes the opposite (and probably correct) position that even > if we decide not to create the extension, we should still fire command > triggers. In a similar vein, AlterFunctionOwner_oid() skips firing > the command triggers if the old and new owners happen to be the same, > but other forms of ALTER FUNCTION (e.g. ALTER FUNCTION .. COST) fire > triggers even if the old and new parameters are the same; and > AlterForeignDataWrapperOwner_internal does NOT skip firing command > triggers just because the old and new owners are the same. We integrate here with the code as it stands, I didn't question the error management already existing. Do we need to revise that in the scope of the command triggers patch? > 4. RemoveRelations() and RemoveObjects() also take the position that a > statement like "DROP TABLE foo, bar" should fire each relevant BEFORE > command trigger twice, then drop both objects, then fire each relevant > AFTER command trigger twice. I think that's wrong. It's 100% clear See above, it's what users are asking us to implement as a feature. > 5. It seems desirable for BEFORE command triggers to fire at a > consistent point during command execution, but currently they don't. The policy should be to fire the triggers once the usual error checking is done. > For example, BEFORE DROP VIEW triggers don't fire until we've verified > that q exists, is a view, and that we have permission to drop it, but > LOAD triggers fire much earlier, before we've really checked anything > at all. And ALTER TABLE is somewhere in the middle: we fire the > BEFORE trigger after checking permissions on the main table, but > before all permissions checks are done, viz: I will rework LOAD, and ALTER TABLE too, which is more work as you can imagine, because now we have to insinuate the command trigger calling into each branch of what ALTER TABLE is able to implement. > 6. Another pretty hideous aspect of the CREATE TABLE behavior is that > AFTER triggers are fired from a completely different place than BEFORE > triggers. It's not this patch's fault that our CREATE TABLE and > ALTER TABLE code is a Rube Goldberg machine, but finding some place to > jam each trigger invocation in that happens to (mostly) work as the > code exists today is not sufficiently future-proof. I'm willing to make it better, what are your ideas? > command we fire the after triggers from ProcessUtility(). That way, > no matter how anybody changes the code in the future, we're guaranteed > that AFTER triggers will always fire exactly once per command, and the From last year's cluster hacker meeting then several mails on this list, it appears that we don't want to do it the way you propose, which indeed would be simpler to implement. > the other hand, the more chance that things will error out before we > even reach the command trigger firing site. I think this is going to I think that's a feature. You skip calling the commands trigger when you know you won't run the command at all. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
