Re: copyParamList

On Wed, Jul 27, 2016 at 2:03 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> 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.
>

Okay, that makes sense to me apart from minor issue reported by
Andrew. I think we might want to slightly rephrase the comments on
top of copyParamList() which indicates only about dynamic parameter
hooks.

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