Re: Command Triggers patch v18

On Tue, Mar 27, 2012 at 4:05 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> > Looking up oids and such before calling the trigger doesn't seem to be
>> > problematic from my pov. Command triggers are a superuser only facility,
>> > additional information they gain are no problem.
>> > Thinking about that I think we should enforce command trigger functions
>> > to be security definer functions.
>> I don't see any benefit from that restriction.
> The reason I was thinking it might be a good idea is that they get might get
> more knowledge passed than the user could get directly otherwise. Especially
> if we extend the scheme to more places/informations.

As long as the superuser gets to decide whether or not a given
function is installed as a command trigger, there's no harm in
allowing him to make it either SECURITY INVOKER or SECURITY DEFINER.
I agree that making it SECURITY DEFINER will often be useful and
appropriate; I just see no reason to enforce that restriction
programatically.

> What are the phases we have:
>
> * after parse
>  * logging
> * after resolving name
> * after checking permisssions  (interwined with the former)
>  * override permission check? INSTEAD?
> * after locking (interwined with the former)
>  * except it needs to be connected to resolving the name/permission check
> this doesn't seem to be an attractive hook point
> * after validation
>  * additional validation likely want to hook here
> * after execution
>  * replication might want to hook here
>
> Am I missing an interesting phase?

I'd sort of categorize it like this:

- pre-parse
- post-parse
- at permissions-checking time
- post permissions-checking/name-lookup, before starting the main work
of the command, i.e. pre-execution
- "event" type triggers that happen when specific actions are
performed (e.g. CREATE, DROP, etc.) or as subcommands fire (e.g. in
ALTER TABLE, we could fire an alter-table-subcommand trigger every
time we execute a subcommand)
- post-execution

> Allowing all that probably seems to require a substantial refactoring of
> commands/ and catalog/

Probably. But we don't need to allow it all at once. What we need to
do is pick one or two things that are relatively easily done,
implement those first, and then build on it. Pre-parse, post-parse,
CREATE-event, DROP-event, and post-execution hooks are all pretty easy
to implement without major refactoring, I think. OTOH,
post-permissions-checking-pre-execution is going to be hard to
implement without refactoring, and ALTER-event hooks are going to be
hard, too.

> I think you need a surprisingly high amount of context to know when to ignore
> a trigger. Especially as its not exactly easy to transfer knowledge from one
> to the next.

I'm not convinced, but maybe it would be easier to resolve this in the
context of a specific proposal.

> I don't think creating *new* DDL from the parsetree can really count as
> statement based replication. And again, I don't think implementing that will
> cost that much effort.
> How would it help us to know - as individual events! - which tuples where
> created where? Reassembling that will be a huge mess. I basically fail to see
> *any* use case besides access checking.

I think if you'd said this to me two years ago, I would have believed
you, but poking through this code in the last year or two, I've come
to the conclusion that there are a lot of subtle things that get
worked out after parse time, during execution. Aside from things like
recursing down the inheritance hierarchy and maybe creating some
indexes or sequences when creating a table, there's also subtle things
like working out exactly what index we're creating: name, access
method, operator class, collation, etc. And I'm pretty sure that if
we examine the code carefully we'll find there are a bunch more.

> I fear a bit that this discussion is leading to something thats never going to
> materialize because it would require a huge amount of work to get there.

Again, the way to avoid that is to tackle the simple cases first and
then work on the harder cases after that, but I don't think that's
what the current patch is doing.

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