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 hooks. -- 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: http://www.postgresql.org/mailpref/pgsql-hackers