Re: [HACKERS] copyParamList

[Robert Haas]

On Wed, Jul 27, 2016 at 10:09 AM, Amit Kapila  wrote:
> On Wed, Jul 27, 2016 at 6:46 PM, Robert Haas  wrote:
>> On Wed, Jul 27, 2016 at 2:20 AM, Amit Kapila  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


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

Re: [HACKERS] copyParamList

[Amit Kapila]

On Wed, Jul 27, 2016 at 6:46 PM, Robert Haas  wrote:
> On Wed, Jul 27, 2016 at 2:20 AM, Amit Kapila  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


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

Re: [HACKERS] copyParamList

[Robert Haas]

On Wed, Jul 27, 2016 at 2:20 AM, Amit Kapila  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


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

Re: [HACKERS] copyParamList

[Robert Haas]

On Wed, Jul 27, 2016 at 12:25 AM, Andrew Gierth
 wrote:
>> "Robert" == Robert Haas  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


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

Re: [HACKERS] copyParamList

[Amit Kapila]

On Wed, Jul 27, 2016 at 2:03 AM, Robert Haas  wrote:
> On Fri, Jul 8, 2016 at 2:28 AM, Amit Kapila  wrote:
>> On Tue, May 31, 2016 at 10:10 PM, Robert Haas  wrote:
>>> On Fri, May 27, 2016 at 6:07 PM, Andrew Gierth
>>>  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

Re: [HACKERS] copyParamList

[Andrew Gierth]

> "Robert" == Robert Haas  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)


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

Re: [HACKERS] copyParamList

[Robert Haas]

On Fri, Jul 8, 2016 at 2:28 AM, Amit Kapila  wrote:
> On Tue, May 31, 2016 at 10:10 PM, Robert Haas  wrote:
>> On Fri, May 27, 2016 at 6:07 PM, Andrew Gierth
>>  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


fix-copyparamlist-rmh.patch
Description: application/download

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

Re: [HACKERS] copyParamList

[Amit Kapila]

On Tue, May 31, 2016 at 10:10 PM, Robert Haas  wrote:
> On Fri, May 27, 2016 at 6:07 PM, Andrew Gierth
>  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


set-parammask-copyparam-v1.patch
Description: Binary data

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

Re: [HACKERS] copyParamList

[Robert Haas]

On Fri, May 27, 2016 at 6:07 PM, Andrew Gierth
 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


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

[HACKERS] copyParamList

[Andrew Gierth]

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)


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