Re: Command Triggers

Hi,

First answer now, new patch version tomorrow.

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> That is better.

Cool :)

> 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.

I guess it's another spelling for a catalog cache, so I'll look at what
it takes to build either a full catcache or this array cache you're
talking about tomorrow.

> 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.

Means I need to update my master's branch and merge conflicts. You could
also test right from my github branch too, I guess.

https://github.com/dimitri/postgres
https://github.com/dimitri/postgres/tree/command_triggers

> 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.

We could decide command triggers are superuser only. Security is not
something I'm very strong at, so I'll leave it up to you to decide.

> 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.

Good catch, I guess we need to remove creating and dropping a command
trigger to the list of supported commands in the ANY COMMAND list.

> 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.

Ah yes, that too was on the TODO list, I just forgot about it. I still
remember the merge conflicts when that patch went it, you know… :)

> 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 we should limit the support to BEFORE command trigger only. It
was unclear to me how to solve the problem for the AFTER command case,
and if you're unclear to, then there's not that many open questions.

> 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

I have no strong feeling about that. Would that require that COMMAND be
more reserved than it currently is (UNRESERVED_KEYWORD)?

> 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.

The other thing is that you might want to drop the trigger from only one
command, of course we could support both syntax. We could also add
support for the following one:

DROP TRIGGER bob ON ALL COMMANDS;

As ALL is already a reserved word, that will work.

> + 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.

Ok, let's see if we keep the feature open to database owners then.

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

Then you have to either make the current command context a backend
private global variable, or amend even more call sites to pass it down.
The global variable idea does not work, see RemoveRelations() and
RemoveObjects() which are handling an array of command contexts.

So do you prefer lots of InitCommandContext() or adding another parameter
to almost all functions called by standard_ProcessUtility()?

> 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.

Merge or diff issue I think, I will look into that, I don't know.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support