Re: Command Triggers patch v18

Apropos of nothing and since I haven't found a particularly good time
to say this in amidst all the technical discussion, I appreciate very
much all the work you've been putting into this.

On Thu, Mar 29, 2012 at 12:42 PM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
>> serious issues in the discussion we've had so far, especially (1) the
>> points at which figures are fired aren't consistent between commands,
>> (2) not much thought has been given to what happens if, say, a DDL
>> trigger performs a DDL operation on the table the outer DDL command is
>> due to modify, and (3) we are eventually going to want to trap a much
>> richer set of events than can be captured by the words "before" and
>> "after".  Now, you could view this as me throwing up roadblocks to
>> validate my previously-expressed opinion that this wasn't going to get
>> done, but I really, honestly believe that these are important issues
>> and that getting them right is more important than getting something
>> done now.
>
> (1) is not hard to fix as soon as we set a policy, which the most
>    pressing thing we need to do in my mind, whatever we manage to
>    commit in 9.2.

I agree that setting a policy is enormously important, not so sure
about how easy it is to fix, but we shall see.

My primary and overriding goal here is to make sure that we don't make
any design decisions, in the syntax or otherwise, that foreclose the
ability to add policies that we've not yet dreamed of to future
versions of the code. It seems vanishingly unlikely that the first
commit will cover all the use cases that people care about, and only
slightly more likely that we'll even be able to list them all at that
point. Perhaps I'm wrong and we already know what they are, but I'd
like to bet against any of us - and certainly me - being that smart.

In terms of policies, there are two that seems to me to be very clear:
at the very beginning of ProcessUtility, and at the very end of it,
*excluding* recursive calls to ProcessUtility that are intended to
handle subcommands. I think we could call the first start or
command_start or ddl_command_start or post-parse or pre-locking or
something along those lines, and the second end or command_end or
ddl_command_end. I think it might be worth including "ddl" in there
because the scope of the current patch really is just DDL (as opposed
to say DCL) and having a separate set of firing points for DCL does
not seem dumb. If we ever try to do anything with transaction control
as you suggested upthread that's likely to also require special
handling. Calling it, specifically, ddl_command_start makes the scope
very clear.

> (2) can be addressed with a simple blacklist that would be set just
>    before calling user defined code, and cleaned when done running it.
>    When in place the blacklist lookup is easy to implement in a central
>    place (utility.c or cmdtrigger.c) and ereport() when the current
>    command is in the blacklist.
>
>    e.g. alter table would blacklist alter table and drop table commands
>    on the current object id

Here we're talking about a firing point that we might call
ddl_post_name_lookup or something along those lines. I would prefer
to handle this by making the following rules - here's I'm assuming
that we're talking about the case where the object in question is a
relation:

1. The trigger fires immediately after RangeVarGetRelidExtended and
before any other checks are performed, and especially before any
CheckTableNotInUse(). This last is important, because what matters is
whether the table's not in use AFTER all possible user-supplied code
is executed. If the trigger opens or closes cursors, for example,
what matters is whether there are any cursors left open *after* the
triggers complete, not whether there were any open on entering the
trigger. The same is true of any other integrity check: if there's a
chance that the trigger could change something that affects whether
the integrity constraint gets fired, then we'd better be darn sure to
fire the trigger before we check the integrity constraint.

2. If we fire any triggers at this point, we CommandCounterIncrement()
and then re-do RangeVarGetRelidExtended() with the same arguments we
passed before. If it doesn't return the same OID we got the first
time, we abort with some kind of serialization error. We need to be a
little careful about the phrasing of the error message, because it's
possible for this to change during command execution even without
command triggers if somebody creates a table earlier in the search
path with the same unqualified name that was passed to the command,
but I think it's OK for the existence of a command-trigger to cause a
spurious abort in that very narrow case, as long as the error message
includes some appropriate weasel language. Alternatively, we could
try to avoid that corner case by rechecking only that a tuple with the
right OID is still present and still has the correct relkind, but that
seems like it might be a little less bullet-proof for no real
functional gain.

There is some difficulty with non-relation objects because we don't
really do proper locking there. For those we should, I think,
rearrange things so that the permissions check happens right after the
name lookup if it doesn't already, and then have the
ddl_post_name_lookup trigger fire right after that. After firing the
triggers we should redo the name lookup and complain if the result has
changed. If somebody improves the locking there in the future, these
cases will become fully symmetrical with the above.

As compared with your proposed blacklist solution, I think this has
the advantage of (1) being more localized in terms of code
modification and (2) being slightly more bullet-proof. For example,
suppose somebody manually deletes a row out of pg_class from the
trigger. Yeah, I know they shouldn't do that, and yeah, it's not our
job to make those cases work, but if we can tolerate them better for
the same amount of work (or less) then I'd rather do it.

> (3) we need to continue designing that, yes. I think we can have a first
>    set of events defined now, even if we don't implement support for
>    all of them readily.

No argument.

>> Parenthetically, what Dimitri previously called the after-any-command
>> firing point, all the way at the end of the statement but without any
>> specific details about the object the statement operated on, seems
>> just as good for a first step, maybe better, so that would be a fine
>> foundation from my point of view as well.  The stuff that happens
>
> Now that fetching the information is implemented, I guess that we could
> still provide for it when firing event trigger at that timing spec. Of
> course that means a bigger patch to review when compared to not having
> the feature, but Thom did spend loads of time to test this part of the
> implementation.

I agree that end-of-command triggers should be able to get a full
report about what happened during the command. I think that will take
a bunch more work to get right, even though you've whittled down the
scope of what information is to be provided quite a bit. I think it's
important to get some of the other things we've been talking about
(see above) hammered out first, so I haven't spent a lot of time yet
figuring out exactly what I think might break in the current
implementation, but I'm going to analyze it more once we've got the
basics done. It's worth noting that this can be added in a
backward-compatible way - i.e. if PG 9.x supports end-of-command
triggers that don't get any information besides the command tag, PG
9.(x+1) can include set a bunch of magic variables to provide that
information. I'm glad you ended up revising things to use the
magic-variable approach rather than a list of specific parameters for
just this reason.

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