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
