Re: copyParamList

On Fri, Jul 8, 2016 at 2:28 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> 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.

Hmm, I don't think this is the right fix. The point of the
bms_is_member test is that we don't want to copy any parameters that
aren't needed; but if we avoid copying parameters that aren't needed,
then it's fine for the new ParamListInfo to have a paramMask of NULL.
So I think it's actually correct that we set it that way, and I think
it's better than saving a pointer to the the original paramMask,
because (1) it's probably slightly faster to have it as NULL and (2)
the old paramMask might not be allocated in the same memory context as
the new ParamListInfo. So I think we instead ought to fix it as in
the attached.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company