Re: Incorrect processing of CREATE TRANSFORM with DDL deparding

On Tue, May 26, 2015 at 10:49 AM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com
> wrote:

> On Tue, May 26, 2015 at 6:47 AM, Michael Paquier 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 got a second look at this code this morning and I think that the
> handling of InvalidObjectAddress is incorrect. For example, running a
> CTAS IF NOT EXISTS on a object that already exists will make
> ExecCreateTableAs return InvalidObjectAddress, and then the
> EventTriggerCollectSimpleCommand() tries to register an event based on
> it. Also, commandCollected is actually not necessary if we rely on the
> fact that address is initialized as InvalidObjectAddress.
>
> The patch attached simplifies the code accordingly, making it more
> readable IMO for the utility.c part. Thoughts?
>

Attached is an updated patch with some tests for test_ddl_deparse. Creating
a TRANSFORM is actually possible with the SQL language (bug?) by using some
of the in-core system functions. The important point being that the FROM
SQL function uses internal as argument, and returns internal, and that the
TO SQL function uses internal as argument, and returns a result of the data
type of the transform.

Hence, something like that works:
+CREATE TRANSFORM FOR int LANGUAGE SQL (
+ FROM SQL WITH FUNCTION varchar_transform(internal),
+ TO SQL WITH FUNCTION int4recv(internal));
That's grotty, OK, but the point is to test the code path for TRANSFORMS.
We could as well create some dummy functions directly in
test_ddl_deparse.c. Let me know if this would work better, but I

And I actually bumped into a bug with this fake test case:
DROP TRANSFORM FOR int LANGUAGE SQL;
+ ERROR: requested object address for unsupported object class 32: text
result "unrecognized object 3576 16551 0"
What happens is that the handling for transforms in getObjectIdentityParts
is missing. This is fixed as well in the attached.

Regards,
--
Michael