Re: Event Triggers: adding information

On 2013-01-18 11:42:47 -0500, Robert Haas wrote:
> On Fri, Jan 18, 2013 at 10:47 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> >> No, there's one also in heap_create_with_catalog.  Took me a minute to
> >> find it, as it does not use InvokeObjectAccessHook.  The idea is that
> >> OAT_POST_CREATE fires once per object creation, regardless of the
> >> object type - table, column, whatever.
> >
> > Ah. Chose the wrong term to grep for. I am tempted to suggest adding a
> > comment explaining why we InvokeObjectAccessHook isn't used just for
> > enhanced grepability.
> 
> I cannot parse that sentence, but I agree that the ungreppability of
> it is suboptimal.  I resorted to git log -SInvokeObjectAccessHook -p
> to find it.

I was thinking of adding a comment like
/* We don't use InvokeObjectAccessHook here because we need to ... */
not because the comment in itself is important but because it allows to
grep ;)


> >> I have been involved in PostgreSQL development for about 4.5 years
> >> now.  This is less time than many people here, but it's still long
> >> enough to say a whole lot of people ask for some variant of this idea,
> >> and yet I have yet to see anybody produce a complete, working version
> >> of this functionality and maintain it outside of the PostgreSQL tree
> >> for one release cycle (did I miss something?).
> >
> > I don't really think thats a fair argument.
> >
> > For one, I didn't really ask for a libpgdump - I said that I don't see
> > any way to generate re-executable SQL without it if we don't get the
> > parse-tree but only the oid of the created object. Not to speak of the
> > complexities of supporting ALTER that way (which is, as you note,
> > basically not the way to do this).
> 
> Oh, OK.  I see.  Well, I've got no problem making the parse tree
> available via InvokeObjectAccessHook.  Isn't that just a matter of
> adding a new hook type and a new call site?

Sure. Its probably not easy to choose the correct callsites though, they
need to be somewhat different for create & alter in comparison to drop.

create & alter
* after the object is created/altered
drop:
* after the object is locked, but *before* its dropped

As far as I understood thats basically the behind the ddl_trace event
trigger mentioned earlier.

> > Also, even though I don't think its the right way *for this*, I think
> > pg_dump and pgadmin pretty much prove that it's possible?  The latter
> > even is out-of-core and has existed for multiple years.
> 
> Fair point.

Imo its already a good justification for a libpgdump, not that I am
volunteering, but ...

> > Its also not really fair to compare out-of-tree effort of maintaining
> > such a library to in-core support. pg_dump *needs* to be maintained, so
> > the additional maintenance overhead once the initial development is done
> > shouldn't really be higher than now. Lower if anything, due to getting
> > rid of a good bit of ugly code ;). There's no such effect out of core.
> 
> This I don't follow.  Nothing we might add to core to reverse-parse
> either the catalogs or the parse tree is going to make pg_dump go
> away.

I was still talking about the hypothetical libpgdump we don't really
need for this ;)

> > If youre also considering parsetree->SQL transformation under the
> > libpgdump umbrella its even less fair. The backend already has a *lot*
> > of infrastructure to regenerate sql from trees, even if its mostly
> > centered arround around DQL. A noticeable amount of that code is
> > unavailable to extensions (i.e. many are static funcs).
> > Imo its completely unreasonable to expose that code to the outside and
> > expect it to have a stable interface. We *will* need to rewrite parts of
> > that regularly.
> > For those reasons I think the only reasonable way to create textual DDL
> > is in the backend trying to allow outside code to do that will impose
> > far greater limitations.
> 
> I'm having trouble following this.  Can you restate?  I wasn't sure
> what you meant by libpqdump.  I assumed you were speaking of a
> parsetree->DDL or catalog->DDL facility.

Yea, that wasn't really clear, sorry for that.

What I was trying to say that I think doing parstree->text conversion
out of tree is noticeably more complex and essentially places a higher
maintenance burden on the project because
1) the core already has a lot of infrastructure for such conversions
2) any external project would either need to copy that infrastructure or  make a large number of functions externally
visible
3) any refactoring in that area would now break external tools even if  it would be trivial to adjust potential in-core
supportfor such a  conversion
 

> > Some version of the event trigger patch contained partial support for
> > regenerating the DDL so it seems like a justified point there. Your
> > complained that suggestions about reusing object access hooks were
> > ignored, so mentioning that they currently don't provide *enough* (they
> > *do* provide a part, but it doesn't seem to be the most critical one)
> > also seems justifyable.
> 
> Sure, but we could if we wanted decide to commit the partial support
> for regenerating the DDL and tell people to use it via object access
> hooks.

Yes, thats a possibility.

> Of course, that would thin out even further the number of
> people who would actually be using that code, which I fear will
> already be too small to achieve bug-free operation in a reasonable
> time.

Well, replication solutions seem to be the main potential consumer of
such a capability and while the C level stuff wouldn't be used by many,
hopefully the stuff built on top will.

> If we add some hooks now and someone maintains the DDL
> reverse-parsing code as an out-of-core replication solution for a few
> releases, and that doesn't turn out to be hideous, I'd be a lot more
> sanguine about incorporating it at that point.

As I said above, I think due to the available infrastructure doing this
out of tree is *far* harder and I fear that a good part of that code
would turn out not to be applicable for inclusion into core.

> I basically don't
> think that there's any way we're going to commit something along those
> lines now and not have it turn out to be a far bigger maintenance
> headache than anyone wants.  What that tends to end up meaning in
> practice is that Tom gets suckered into fixing it because nobody else
> can take the time, and that's neither fair nor good for the project.

Now as is in "at this point of the cycle" or now as in "without a
several year old out-of-core project"?

> > NP, its good to keep the technical stuff here anyway... Stupid
> > technology. In which business are we in again?
>
> I don't know, maybe we can figure it out based on the objects in our
> immediate vicinity.  I see twelve cans of paint, most of them opened,
> and something that looks sort of like a badly-made shed.  Does that
> help at all?  :-)

By the smell and looks from where I am sitting I seem to be a barrista
making rather deliciously smelling coffee and my boss seems to suck at
decorating ;). Oh, and I seem to have a terrible taste in music.

Regards,

Andres
-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services