Re: Last gasp - Mailing list pgsql-hackers

On Thu, Apr 5, 2012 at 3:56 PM, Hannu Krosing <hannu@krosing.net> wrote:
> On Thu, 2012-04-05 at 15:30 -0400, Robert Haas wrote:
>> On Thu, Apr 5, 2012 at 3:20 PM, Hannu Krosing <hannu@krosing.net> wrote:
>> > For me it would be enough to know if the trigger is fired by top-level
>> > command or not.
>>
>> Well, you would have been out of luck, then.
>
> It seemed to me you were coming along nicely with fixing that by adding
> the "toplevel", except that the disussion drifted away to solving some
> bigger problems of DDL triggering DDL.

Well, here's the basic issue.  If you want to trigger at the very
beginning or very end of a DDL command, that's really not that hard.
It can be done just by modifying ProcessUtility.  And just to be
clear, by "not very hard" what I really mean is "you could get it done
in 2 CommitFests if you are a talented and experienced hacker".

The problem is that what Dimitri really wants, at least in part, is
command triggers that fire in the *middle* of commands - say, just
after locking the object we're performing surgery on.  That turns out
to be a lot harder than it looks for reasons that are NOT Dimitri's
fault.  We tend to assume that PostgreSQL's code is basically good
code, and in general that's true, but some of the code that is at
issue here is actually really bad code.  It uses ugly, nasty hacks to
get the different things that have to happen as part of a DDL
operation to happen in the right order, and it's not very careful
about doing locking, name lookups, and permissions checking in one and
only one place.  This means that if you stick a hook in the right
place to fire triggers for command A, you actually end up firing the
triggers as part of commands B, C, and D when they reach under the
hood and trick the function that runs command A into thinking that
it's executing the toplevel command A is supposed to process when, in
reality, we're doing something completely different and just using
that function as a subroutine.  It is probably not a tremendous amount
of work to refactor ourselves out of this problem, but that work
hasn't been done yet.

There's a second problem, too, which is that not every place in the
backend is a safe place to execute arbitrary user-defined code.  Now,
the approach you are proposing to that problem is to say, hey, if it
crashes the backend, then don't do it that way.  And obviously, with
functions written in C, we have no choice about that; native code can
crash the backend whether we like it or not.  However, I think that we
rightly have a higher standard for features that are exposed at the
SQL level.  It is likely true that if you or I or Dimitri were using
this feature, we could tiptoe our way around doing anything that was
dangerous, and we probably wouldn't be very confused by the strange
trigger-firing behavior either, because we understand how the code is
written.  I don't think our users will be so forgiving.  They right
expect that the behavior will be logical and documented, that it won't
eat their data or crash without a good reason, and that when things do
go wrong they'll get an intelligible error message.  The current
approach doesn't guarantee any of those things.

>> Whether or not it works is one thing.  I think it's acceptable for it
>> to not do anything useful, although actually I think that given a week
>> to work on it I could make it to completely safe.
>
> Due to fractal nature of programming problems I doubt the "completely
> safe" part  ;)
>
> You (or someone else) would have found a next obscure set of conditions
> where specially crafted function and/or schema could wreak havoc
> somewhere.

Well, it's not possible to write completely perfect code for anything
other than a completely trivial program, and I think there is
certainly a time to say, hey, you know, this patch may not be perfect
in every way, but it is good enough that we should start with this as
a base, and then build on it.  But I think that forseeable catalog or
data corruption hazards and anything that might pop out an elog() are
generally accepted as things that we want to avoid even in initial
commits.

> I really should have started panicking earlier.

Yep.  I think Tom and I and others were all pretty clear that there
were not enough people reviewing this CommitFest, and that we were
spread pretty thin trying to make sure every patch got at least some
attention.  There were many patches that I could have done more with
if I hadn't had >30 of them to do something with, and this is one of
them.  I could have looked at it sooner; I could have spent time
revising it.  Your position is apparently that since I noted problems
with the patch, it was my job to fix it.  I don't know how to square
that with the seemingly-universal assumption that every patch is
entitled to get looked at by a committer.  I cannot both look at every
patch and fix every problem I find with each one.  If the lesson we
take from this CommitFest is that the people who put the most time
into making it successful are jerks for not having done even more, I'm
going to find that rather demoralizing.

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