Re: copyParamList

On Tue, May 31, 2016 at 10:10 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, May 27, 2016 at 6:07 PM, Andrew Gierth
> <andrew(at)tao11(dot)riddles(dot)org(dot)uk> wrote:
>> copyParamList does not respect from->paramMask, in what looks to me like
>> an obvious oversight:
>>
>> retval->paramMask = NULL;
>> [...]
>> /* Ignore parameters we don't need, to save cycles and space. */
>> if (retval->paramMask != NULL &&
>> !bms_is_member(i, retval->paramMask))
>>
>> retval->paramMask is never set to anything not NULL in this function,
>> so surely that should either be initializing it to from->paramMask, or
>> checking from->paramMask in the conditional?
>
> Oh, dear. I think you are right. I'm kind of surprised this didn't
> provoke a test failure somewhere.
>

The reason why it didn't provoked any test failure is that it doesn't
seem to be possible that from->paramMask in copyParamList can ever be
non-NULL. Params formed will always have paramMask set as NULL in the
code paths where copyParamList is used. I think we can just assign
from->paramMask to retval->paramMask to make sure that even if it gets
used in future, the code works as expected. Alternatively, one might
think of adding an Assert there, but that doesn't seem to be
future-proof.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com