Re: Command Triggers

On Thu, Feb 16, 2012 at 12:42 PM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> Fixed, I've spent quite some time refining the API.

That is better.

What could still be done here is to create a cache (along the lines of
attoptcache.c) that stores a big array with one slot for each
supported command type. The first time a command is executed in a
particular session, we construct a cache entry for that command type,
storing the OIDs of the before and after triggers. Then, instead of
having to do a catalog scan to notice that there are no triggers for a
given command type, we can just reference into the array and see, oh,
we probed before and found no triggers. CacheRegisterSyscacheCallback
or some other mechanism can be used to flush all the cached
information whenever we notice that pg_cmdtrigger has been updated.

Now, I don't know for sure that this is necessary without performance
testing, but I think we ought to try to figure that out. This version
doesn't apply cleanly for me; there is a conflict in functioncmds.c,
which precludes my easily benchmarking it.

It strikes me that this patch introduces a possible security hole: it
allows command triggers to be installed by the database owner, but
that seems like it might allow the database owner can usurp the
privileges of any user who runs one of these commands in his or her
database, including the superuser. Perhaps that could be fixed by
running command triggers as the person who created them rather than as
the superuser, but that seems like it might be lead to other kinds of
privilege-escalation bugs.

If I install a command trigger that prevents all DDL, how do I
uninstall it? Or say I'm the superuser and I want to undo something
my disgruntled DBA did before he quit.

I would much prefer to have DropCmdTrigStmt wedge itself into the
existing DropStmt infrastructure which I just recently worked so hard
on. If you do that, you should find that you can then easily also
support comments on command triggers, security labels on command
triggers (though I don't know if that's useful), and the ability to
include command triggers in extensions.

I am a bit worried about supporting command triggers on statements
that do internal transaction control, such as VACUUM and CREATE INDEX
CONCURRENTLY. Obviously that's very useful and I'd like to have it,
but there's a problem: if the AFTER trigger errors out, it won't undo
the whole command. That might be very surprising. BEFORE triggers
seem OK, and AFTER triggers might be OK too but we at least need to
think hard about how to document that.

I think it would be better to bail on trying to use CREATE TRIGGER and
DROP TRIGGER as a basis for this functionality, and instead create
completely new toplevel statements CREATE COMMAND TRIGGER and DROP
COMMAND TRIGGER. Then, you could decide that all command triggers
live in the same namespace, and therefore to get rid of the command
trigger called bob you can just say "DROP COMMAND TRIGGER bob",
without having to specify the type of command it applies to. It's
still clear that you're dropping a *command* trigger because that's
right in the statement name, whereas it would make me a bit uneasy to
decide that "DROP TRIGGER bob" means a command trigger just by virtue
of the fact that no table name was specified. That would probably
also make it easier to accomplish the above-described goal of
integrating this into the DropStmt infrastructure.

+ if (!superuser())
+ if (!pg_database_ownercheck(MyDatabaseId, GetUserId()))
+ aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_DATABASE,
+
get_database_name(MyDatabaseId));

The separate superuser check is superfluous; pg_database_ownercheck()
already handles that.

Can we call InitCommandContext in some centralized place, rather than
separately at lots of different call sites?

I am confused why this is adding a new file called dumpcatalog.c which
looks suspiciously similar to some existing pg_dump code I've been
staring at all day.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company