Re: Event Triggers: adding information - Mailing list pgsql-hackers

Robert Haas <robertmhaas@gmail.com> writes:
> - adds ddl_command_trace and ddl_command_end events
>
> I think ddl_command_end is OK and I'm willing to commit that if
> extracted as its own patch.  I think ddl_command_trace is unnecessary
> syntactic sugar.

Ok. Will prepare a non controversial patch for ddl_command_end. After
having played with the patch, I happen to quite like ddl_command_trace,
but I won't try to protect it from its death any more than that.

> - causes those events to be called not only for actual SQL commands
> but also for things that happen, at present, to go through the same
> code path
>
> I think this is a bad idea, not only because, as I said before, it
> exposes internal implementation details, but also because it's
> probably not safe.  As Noah (I believe, or maybe Tom), observed in a
> previous post, we've gone to a lot of work to make sure that nothing
> you do inside a trigger can crash the server, corrupt the catalogs,
> trigger elog() messages, etc.  That applies in spades to DDL.

I naively though that doing CommandCounterIncrement() before running the
event trigger would go a long enough way towards making the operation
safe when the current code is calling back into ProcessUtility().

> Despite Tom's skepticism, I'm pretty sure that it's safe for us to
> execute trigger function calls either completely before or completely
> after we execute the DDL command.  It should be no different than
> executing them as separate commands.  But doing something in the
> middle of a command is something else altogether.  Consider, for
> example, that ALTER TABLE does a bunch of crap internally to itself,
> and then recurses into ProcessUtility to do some more stuff.  This is
> a terrible design, but it's what we've got.  Are you absolutely sure
> that the intermediate state that exists after that first bunch of
> stuff is done and before those other things are done is a safe place
> to execute arbitrary user-provided code?  I'm not.  And the more
> places we add that recurse into ProcessUtility, or indeed, add event
> triggers in general, the more places we're going to add new failure
> modes.

While I hear you, I completely fail to understand which magic is going
to make your other idea safer than this one. Namely, calling an event
trigger named sql_drop rather than ddl_command_start from the same place
in the code is certainly not helping at all.

If your answer to that is how to implement it (e.g. keeping a queue of
events for which to fire Event Triggers once we reach a safe point in
the code), then why couldn't we do exactly the same thing under the
other name?

> Fixing that is part of the price of adding a new event trigger and I'm
> OK with that.  But I'm not OK with baking into the code the assumption
> that any current or future callers of ProcessUtility() should be
> prepared for arbitrary code execution.  The code wasn't written with
> that in mind, and it will not end well.

Again, while I agree with the general principle, it happens that the
calls to the arbitrary code are explicit in ProcessUtility(), so that if
the place is known to be unsafe it's easy enough to refrain from calling
the user code. We do that already for CREATE INDEX CONCURRENTLY at
ddl_command_end.

> - adds additional magic variables to PL/pgsql to expose new
> information not previously exposed
>
> I'm not sure that I agree with the implementation of this, but I think
> I'm OK with the concept.  I will review it when it is submitted in a
> form not intertwined with other things.

Fair enough.

> - adds CONTEXT as an additional event-trigger-filter variable, along with TAG
>
> I'm OK with adding new event-trigger-filter variables, provided that
> they are done without additional run-time overhead for people who are
> not using event triggers; I think it's nice that we can support that.
> But I think this particular proposal is DOA because nothing except
> TOPLEVEL is actually safe.

Yeah, let's first see about that.

>> My thinking is that the user code can do anything, even run a DROP
>> command against the object that we are preparing ourselves to be
>> dropping. I don't think we can bypass doing the lookup twice.
>
> Sure, but there's also the issue of getting the right answer.  For
> example, suppose you are planning to drop a function.  You do a name
> lookup on the name and call the event trigger.  Meanwhile, someone
> else renames the function and creates a new one with the same name.
> After the event trigger returns, the actual drop code latches onto the
> one with the new name.  Now, the event trigger ran with OID A and the
> actual object dropped was one with OID B.  It's possible that KaiGai's

Is your scenario even possible when we take a ShareUpdateExclusiveLock
on he object before running the Event Trigger, in order to protect
against exactly that scenario, as said in the comment? Maybe another
lock should be taken. Which one?

> There's another issue here, too, which is that this change
> reintroduces a failure mode that I spent a lot of time cleaning up
> during the 9.2 cycle - namely, taking locks on a table before
> permissions have been checked, thus providing new methods for
> unprivileged users to block operations on tables they don't have
> rights to.

Aren't Event Triggers superuser only?  Do your concern means that we
should force Event Triggers Functions to be SECURITY DEFINER?

>>  - the parse tree, only available to C code, and without any API
>>    stability promises
>
> I'm OK with that.  People who program in C are taking their life in
> their hands anyway as far as new versions are concerned.

Cool.

> What I'm proposing is that we have a separate set of events that get
> invoked every time an object is created, altered, or dropped,
> regardless of the cause.  Yes, the name will be different, but the
> point is not to only change the name, but rather to also change the
> definition.  There are two separate things you might want here:

In the rest of your email, I can't decide what kind of information you
want those new events to give to the Event Trigger User Code. The other
thing is, as I said before, that I can't see why those new events would
suddenly make the call point safer.

Other than that, I must say that your proposal is sound to me and that I
like it too. I just fail to understand how it makes any difference other
than in naming things, really.

Why would a command that the system executes for you has to be
considered such a different beast from the same command when you happen
to have sent explicitely? A GENERATED context sounds better to me than a
"sql_create" event as a user interface, obviously, and implementing the
later has the *exact* same set of challenges to address, as far as I
understand.

> For example, suppose a user executes the command "DROP OWNED BY fred".
>  For a logging hook, you want the first thing: one callback, saying,
> hey, the user is trying to run DROP OWNED BY fred.  You log it once,
> and go home.  For a replication hook, you want the second thing: one

Maybe it's only me, but I'm really annoyed that there's currently no way
to get the list of objects actually dropped other than parsing the logs.
So I know that I would want my audit trigger to see them all.

> callback per object, with details about each one, so you can drop them
> on the remote side.  Both things are useful, but they are different.

Sure. And you have exactly that feature in my patch, where by default
CONTEXT is TOPLEVEL and you see only the main command, but you can also
register against both TOPLEVEL and GENERATED, say CASCADE in the future,
and have all the details. Or only register against CASCADE, too.

> As a further example, suppose that in 9.4 (or 9.17) we add a command
> "DROP TABLES IN SCHEMA fred WHERE name LIKE 'bob%'".  Well, the
> logging trigger still just works (because it's only writing the
> statement, without caring about its contents).  And the replication
> trigger still just works too (because it will still get a callback for
> every table that gets dropped, even though it knows nothing about the
> new syntax).  That's very powerful.  Of course, there will still be
> cases where things have to change, but you can minimize it by clean
> definitions.

Your use case here is exactly the reason why we proposed to have the
main DROP command implemented via ProcessUtility, so that an Event
Trigger called with a CASCADE context works the same way whatever the
TOPLEVEL command looks like, and always against a single object.

> It pains me that I've evidently failed to communicate this concept
> clearly despite a year or more of trying.  Does that make sense?  Is
> there some way I can make this more clear?  The difference seems very
> clear-cut to me, but evidently I'm not conveying it well.

Communication is hard, and apparently even more so for computer-people,
or they wouldn't prefer to spend their jobs talking to a computer rather
than talking to people. I've heard that so many times that it almost
could make it true, sometimes…

>> After all, the whole point of the Event Trigger patch is to expose some
>> level of implementation details.
>
> I don't really agree with that.  I think the point is to expose what
> the system is doing to the DBA.  I'm OK with exposing the fact that
> creating a table with a serial column also creates a sequence.  There
> is no problem with that - it's all good.  What we DON'T want to do is
> set up a system where the fact that it creates a sequence is exposed
> because that happens to go through ProcessUtility() and the fact that
> it creates a constraint is not because that happens not to go through
> ProcessUtility().  That is not the sort of quality that our users have
> come to expect from PostgreSQL.

I see what you mean. I don't see any CREATE CONSTRAINT command in the
following reference, though.
 http://www.postgresql.org/docs/9.2/static/sql-commands.html

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