Re: Add CREATE support to event triggers - Mailing list pgsql-hackers

On 2014-11-08 10:42:15 -0500, Robert Haas wrote:
> On Sat, Nov 8, 2014 at 4:37 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> > I don't understand why this is particularly difficult to regresssion
> > test. It actually is comparatively simple?
> 
> If it is, great.  I previously wrote this email:
> 
> http://www.postgresql.org/message-id/CA+TgmoZ=vZriJMxLkqi_V0jg4k4LEAPmwUSC6RWXS5MquXUJNA@mail.gmail.com
> 
> Alvaro came up with a way of addressing the second point I raised
> there, which I'm quite pleased about, but AFAIK there's been no
> progress on the first one.  Maybe I missed something?

I unfortunately don't think so. And that sounds like a completely
reasonable criticism.

> Just to illustrate the point, consider the CREATE TABLE name OF type
> syntax that Peter added a few years ago.   That patch
> (e7b3349a8ad7afaad565c573fbd65fb46af6abbe) had the following impact on
> gram.y:
> 
>  src/backend/parser/gram.y |   56 ++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 43 insertions(+), 13 deletions(-)

> Now let's have a look at what impact it has on the deparsing code.
> Patch 6 has deparse_ColumnDef_Typed from lines 134 to 193.  There's 3
> or so lines in deparseTableElements that decide whether to call it.
> Patch 8 has more handling for this case, lines 439 to 443 and 463 to
> 490.  So, if this feature had been committed before TABLE OF, it would
> have needed about 100 lines of net new code to handle this case -
> exclusive of refactoring.  The actual size of the patch would probably
> have been modestly larger than that, because some code would need to
> be reindented when it got iffed out, and quite possibly some
> rearrangement would have been needed.  But even ignoring all that, the
> deparse footprint of the patch would have been MORE THAN TWICE the
> parser footprint.

Well, you disregarded the related costs of adjusting pg_dump et al, as
Tom mentioned, that's a significant part. And yes, there's some
additions that aren't entirely trivial to add. But the majority of
additions are pretty simple.

> I think that's going to be typical; and I think the deparse code is
> going to be significantly more labor-intensive to write than bison
> productions are.  Do you really think that's not going to be a burden?

I've looked into a fair number of cases and almost all are vastly
simpler than this. Most of the DDL changes that have been done lately
are things like adding IF NOT EXISTS somewhere; expanding existing
syntax for new types of objects (ALTER ... RENAME for foreign servers,
wrappers; LABEL for new types).

I think given the complexity of newly added features the overhead of
adding deparsing code isn't all that high.

> > Being able to replicate DDL is a feature wish that has been around
> > pretty much since the inception of trigger based replication
> > solution. It's not some current fancy. And the json stuff only got there
> > because people wanted some way to manipulate the names in the replicated
> > - which this abstraction provides them with.
> 
> I understand that being able to replicate DDL is an important need,
> and there may be no better way to do it than this.  But that doesn't
> mean that this code is going to be bug-free or easy to maintain.

Agreed. There's definitely no free lunch (here). This has been discussed
more than once, and so far I've read anything superior that also has a
chance of handling ALTER.

> >> may never be, and which I strongly suspect may be too clever by half.
> >> Once you've committed it, you're going to move onto other things and
> >> leave it to everyone who comes after to try to maintain it.  I bet
> >> we'll still be running into bugs and half-implemented features five
> >> years from now, and maybe in ten.  Ramming through special-purpose
> >> infrastructure instead of generalizing it is merely icing on the cake,
> >> but it's still moving in the wrong direction.
> >
> > You're just as much to blame for not writing a general json abstraction
> > layer for EXPLAIN. I'd say that's not much blame, but still, there's
> > really not much difference there.
> 
> Well, this patch is series is at least an order of magnitude larger,
> and it's apparently doing significantly more complex stuff with JSON,
> because the explain.c patch just writes it out into a StringInfo.

And this code builds up the data in memory and then calls into the json
code to build the json value. And to parse json it uses the functions
underlying the SQL accessors.

There's one function it should either not need anymore (dequote_jsonval)
by using json_object_field_text instead of json_object_field or by
exposing dequote_jsonval's functionality from json.c.

I think the json angle here is a red herring. Sure, a nicer API could
save some FunctionCallInfoData boilerplate, but that's pretty much it.

Greetings,

Andres Freund

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