Обсуждение: copyParamList
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?
--
Andrew (irc:RhodiumToad)
On Fri, May 27, 2016 at 6:07 PM, Andrew Gierth <andrew@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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, May 31, 2016 at 10:10 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Fri, May 27, 2016 at 6:07 PM, Andrew Gierth > <andrew@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. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Вложения
On Fri, Jul 8, 2016 at 2:28 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: > On Tue, May 31, 2016 at 10:10 PM, Robert Haas <robertmhaas@gmail.com> wrote: >> On Fri, May 27, 2016 at 6:07 PM, Andrew Gierth >> <andrew@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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Вложения
>>>>> "Robert" == Robert Haas <robertmhaas@gmail.com> writes:
Robert> So I think we instead ought to fix it as in the attached.
Robert> if (retval->paramMask != NULL &&Robert> - !bms_is_member(i, retval->paramMask))Robert> +
!bms_is_member(i, from->paramMask))
Need to change both references to retval->paramMask, not just one.
--
Andrew (irc:RhodiumToad)
On Wed, Jul 27, 2016 at 2:03 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Fri, Jul 8, 2016 at 2:28 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: >> On Tue, May 31, 2016 at 10:10 PM, Robert Haas <robertmhaas@gmail.com> wrote: >>> On Fri, May 27, 2016 at 6:07 PM, Andrew Gierth >>> <andrew@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
On Wed, Jul 27, 2016 at 12:25 AM, Andrew Gierth <andrew@tao11.riddles.org.uk> wrote: >>>>>> "Robert" == Robert Haas <robertmhaas@gmail.com> writes: > Robert> So I think we instead ought to fix it as in the attached. > > Robert> if (retval->paramMask != NULL && > Robert> - !bms_is_member(i, retval->paramMask)) > Robert> + !bms_is_member(i, from->paramMask)) > > Need to change both references to retval->paramMask, not just one. You are, of course, entirely correct. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Jul 27, 2016 at 2:20 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: > 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. I don't see why it needs to be changed - can you explain in more detail what you have in mind? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Jul 27, 2016 at 6:46 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Wed, Jul 27, 2016 at 2:20 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: >> 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. > > I don't see why it needs to be changed - can you explain in more > detail what you have in mind? > Basically after this function, usage of ParamListInfo doesn't need to care for value of paramMask as you have already ignored the required params. I think it is quite apparent from the code, but as the similar information is mention for dynamic parameter hooks, I thought we might want to update it. If you don't feel the need of same, then leave it as it is. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Wed, Jul 27, 2016 at 10:09 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: > On Wed, Jul 27, 2016 at 6:46 PM, Robert Haas <robertmhaas@gmail.com> wrote: >> On Wed, Jul 27, 2016 at 2:20 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: >>> 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. >> >> I don't see why it needs to be changed - can you explain in more >> detail what you have in mind? >> > > Basically after this function, usage of ParamListInfo doesn't need to > care for value of paramMask as you have already ignored the required > params. I think it is quite apparent from the code, but as the > similar information is mention for dynamic parameter hooks, I thought > we might want to update it. If you don't feel the need of same, then > leave it as it is. Yeah, I don't see a need to mention that. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company