Thread: Incorrect processing of CREATE TRANSFORM with DDL deparding
Hi all, 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. Regards, -- Michael
Attachment
Michael Paquier wrote: > Hi all, > > 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. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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. -- Michael
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? -- Michael
Attachment
* Michael Paquier (michael.paquier@gmail.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.
>=20
> 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
Stephen Frost wrote:
> 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?
That doesn't work. Consider DropStmt for example; it is supported for
some object types, but not supported by others. There are a few other
commands for which this happens too. Also, NodeTag contains tags for
everything that can be a node: plan nodes for example, and for those it
doesn't even make sense to consider whether event triggers are
supported.
I suppose you could create a node type T_UtilityStmt and make all the
command nodes be sub-types of that one. But, except for the
ProcessUtility{Slow} code, it would be a loss in maintainability, ISTM,
and it would increase the size of every single utility node.
Now maybe with that you could get rid of (or centralize) various arrays;
see for example ObjectTypeMap and event_trigger_support, the calls to
pg_strcasecmp() in check_ddl_tag, EventTriggerSupportsObjectType() and
EventTriggerSupportsObjectClass() and
EventTriggerSupportsGrantObjectType() ...
Anyway this would solve the problem at hand anyway.
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro,
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> Stephen Frost wrote:
> > 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?
>=20
> That doesn't work. Consider DropStmt for example; it is supported for
> some object types, but not supported by others. There are a few other
> commands for which this happens too. Also, NodeTag contains tags for
> everything that can be a node: plan nodes for example, and for those it
> doesn't even make sense to consider whether event triggers are
> supported.
Yeah, good points, all of them.
> I suppose you could create a node type T_UtilityStmt and make all the
> command nodes be sub-types of that one. But, except for the
> ProcessUtility{Slow} code, it would be a loss in maintainability, ISTM,
> and it would increase the size of every single utility node.
>=20
> Now maybe with that you could get rid of (or centralize) various arrays;
> see for example ObjectTypeMap and event_trigger_support, the calls to
> pg_strcasecmp() in check_ddl_tag, EventTriggerSupportsObjectType() and
> EventTriggerSupportsObjectClass() and
> EventTriggerSupportsGrantObjectType() ...
>=20
> Anyway this would solve the problem at hand anyway.
I certainly agree that being able to centralize those arrays would be
nice. It has definitely struck me, more than once, that we've got quite
a few different arrays hanging around that might be better combined into
one or at least kept in the same area. If nothing else, it might help
minimize cases where individuals forget to update one or another of the
arrays.
Thanks!
Stephen
On Tue, May 26, 2015 at 10:49 AM, Michael Paquier <michael.paquier@gmail.com> wrote:
I got a second look at this code this morning and I think that theOn 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.
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"
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
Attachment
Michael Paquier wrote: > 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. Oh, interesting. We need one or two extra lines in the object_address.sql test as well, I guess. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Jun 5, 2015 at 6:24 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > > Michael Paquier wrote: > > > 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. > > Oh, interesting. We need one or two extra lines in the > object_address.sql test as well, I guess. Ah, so that's where it is... Attached is an updated patch with this stuff. I noticed a couple of code paths where OCLASS_TRANSFORM was missing as well. -- Michael
Attachment
Michael Paquier wrote: > On Fri, Jun 5, 2015 at 6:24 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > > > > Michael Paquier wrote: > > > > > 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. > > > > Oh, interesting. We need one or two extra lines in the > > object_address.sql test as well, I guess. > > Ah, so that's where it is... Attached is an updated patch with this > stuff. I noticed a couple of code paths where OCLASS_TRANSFORM was > missing as well. Thanks, pushed that part of it. Looking at the event trigger bits next. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, Jun 22, 2015 at 4:25 AM, Alvaro Herrera wrote: > Thanks, pushed that part of it. Looking at the event trigger bits next. Thanks for looking at that. Not sure if that's useful, but attached is a rebased version of the rest. -- Michael
Attachment
Michael Paquier wrote: > On Mon, Jun 22, 2015 at 4:25 AM, Alvaro Herrera wrote: > > Thanks, pushed that part of it. Looking at the event trigger bits next. > > Thanks for looking at that. Not sure if that's useful, but attached is > a rebased version of the rest. Pushed the fixes to transform and the new test file, thanks; didn't push the other changes for ObjectAddressEq() and the changes in ProcessUtilitySlow. These are attached. I don't want to lose that stuff: I would like to have an assertion that verifies that the branch in the large switch has at least given some thought to command collection (precisely to prevent bugs of the type being fixed here.) One problem is cases that return InvalidObjectAddress because of IF NOT EXISTS, for instance, so it's not just a one-line assert addition. I just noticed I neglected to credit for authorship in the commit message, sorry about that :-( -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
On Sat, Jun 27, 2015 at 6:31 AM, Alvaro Herrera wrote: > Michael Paquier wrote: >> On Mon, Jun 22, 2015 at 4:25 AM, Alvaro Herrera wrote: >> > Thanks, pushed that part of it. Looking at the event trigger bits next. >> >> Thanks for looking at that. Not sure if that's useful, but attached is >> a rebased version of the rest. > > Pushed the fixes to transform and the new test file, thanks; didn't push > the other changes for ObjectAddressEq() and the changes in > ProcessUtilitySlow. These are attached. I don't want to lose that > stuff: I would like to have an assertion that verifies that the branch > in the large switch has at least given some thought to command > collection (precisely to prevent bugs of the type being fixed here.) > One problem is cases that return InvalidObjectAddress because of IF NOT > EXISTS, for instance, so it's not just a one-line assert addition. OK, but actually what does it mean to register an event on an invalid object. You cannot get a definition from it, right? Hence you cannot register an event assigned to it. > I just noticed I neglected to credit for authorship in the commit > message, sorry about that :-( Don't worry. That's fine as-is. -- Michael