Re: Command Triggers patch v18

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> 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.

Hey, thanks, I very much appreciate your support here!

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

I think your proposal idea on the table is fixing it (the new event
timing specs), it's all about fine tuning it now.

> 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

Yeah we're not hard coding the list of possible event timings in the
syntax.

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

I'm not sure about that really. First, the only reason why DCL command
are not supported in the current patch is because roles are global
objects and we don't want role related behavior to depend on which
database you're currently connected to.

Then, I guess any utility command implementation will share the same
principle of having a series of step and allowing a user defined
function to get called in between some of them. So adding support for
commands that won't fit in the DDL timing specs we're trying to design
now amounts to adding timing specs for those commands.

I'm not sold that the timing specs should contain ddl or dcl or other
things, really, I find that to be ugly, but I won't fight over that.

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

Ack.

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

I can buy having a special error case here as it's about concurrent DDL
and while we need to get it right, errors are often the best we can do.

Now it seems to me you're already describing the ultimate goal of being
able to recheck the pre-conditions once the user defined code returned,
whereas I'm proposing a blunt tool for version 1 in order to defensively
avoid the class of problems.

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

Again, next version, right? Rearrange things is gigantic code churn.

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

It's more sophisticated, no doubt. It's at least an order of magnitude
more work, too, from the simple fact that it begins with proof reading
all commands implementation to “rearrange things”.

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

Cool :)

> 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

Ok, so I soon will be able to reshuffle the code with the current
design, adding two simple entry points for command triggers and revising
the internal APIs to adjust. Then with some luck we get back the
intermediate call that used to be spelled "before command".

To be able to have the “full report” in the end-of-command case, we will
need to maintain the location from where the command triggers are
called as of now, that is spread in every command implementation.

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

Yeah it's better this way even if it means implementing the same thing
for each pl in core, so 4 times…

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