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

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?

>> and which will likely receive very limited
>> testing from users to support a feature that is not in core,
>
> Just like half of the features you worked on yourself lately? Why is
> this an argument?

Because the stuff I'm adding doesn't break every time someone adds a
new DDL command, something that we do regularly.

If it were a question of writing this code once and being done with
it, that would be unobjectionable in my view.  But it isn't.
Practically every change to gram.y is going to require a corresponding
change to this stuff.  As far as I can see, nobody except me has
commented on the burden that places on everyone who may wish to add
syntax support for a new construct in the future, which might mean
that I'm worrying about something that isn't worth worrying about, but
what I think is more likely is that nobody's worrying about it right
now because they haven't had to do it yet.

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.

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?

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

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

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