On Wed, Jul 27, 2016 at 2:03 AM, Robert Haas <robertmh...@gmail.com> wrote:
> On Fri, Jul 8, 2016 at 2:28 AM, Amit Kapila <amit.kapil...@gmail.com> wrote:
>> On Tue, May 31, 2016 at 10:10 PM, Robert Haas <robertmh...@gmail.com> wrote:
>>> On Fri, May 27, 2016 at 6:07 PM, Andrew Gierth
>>> <and...@tao11.riddles.org.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

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

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to