From: | Stephen Frost <sfrost(at)snowman(dot)net> |
---|---|
To: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com> |
Cc: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, PostgreSQL mailing lists <pgsql-bugs(at)postgresql(dot)org> |
Subject: | Re: Incorrect processing of CREATE TRANSFORM with DDL deparding |
Date: | 2015-05-26 02:02:43 |
Message-ID: | 20150526020243.GU26667@tamriel.snowman.net |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-bugs |
* Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
> On Tue, May 26, 2015 at 12:52 AM, Alvaro Herrera wrote:
> > Michael Paquier wrote:
> >> In ProcessUtilitySlow()@utility.c, for a node T_CreateTransformStmt,
> >> process does not return ObjectAddress. This makes process inconsistent
> >> with the other commands and the ObjectAddress passed to
> >> EventTriggerCollectSimpleCommand is not initialized.
> >> Coverity has pointed out the error, I just some legwork to sort out a fix.
> >
> > Yeah, I had noticed this and was pretty annoyed because we ended up in
> > precisely the situation we didn't want to be: new code is added to
> > ProcessUtility that is not handled by the deparse framework. (I
> > don't know whether TRANSFORM went in first or deparse, but it doesn't
> > really matter.)
> >
> > The fix as you say is pretty trivial, but I would like to use this is a
> > test case to ensure that we will catch all these mistakes in the future
> > too not only this particular one.
>
> I guess that you could add an assertion at the bottom of
> ProcessUtilitySlow() as well to check code paths where ObjectAddress
> is not initialized correctly.
I seem to recall that being in one of the variations of this patch and I
tend to agree that it makes sense...
That said, I'm really not all that happy with the split between
ProcessUtility() and ProcessUtilitySlow(). I've not said anything since
I haven't got any great solutions to the issue, but it really is pretty
grotty. I realize it might take a few extra cycles, but my thinking is
along the lines of having an array or similar which we scan that
indicates what is supported by deparse/event triggers, what isn't, etc,
and then we operate based on that.. Perhaps an array which is indexed
based on the NodeTag enum? I realize that'd be a stupidly large array,
of, I dunno, 8k or more, but it'd surely make ProcessUtility a heck of a
lot shorter/simpler..
Considering the line count between the two is over 1000, perhaps it'd be
a net savings in size, if not speed.
Just a few off-the-cuff thoughts.
Thanks!
Stephen
From | Date | Subject | |
---|---|---|---|
Next Message | Michael Paquier | 2015-05-26 07:01:26 | Re: PQexec() hangs on OOM |
Previous Message | Michael Paquier | 2015-05-26 01:49:49 | Re: Incorrect processing of CREATE TRANSFORM with DDL deparding |