Re: [HACKERS] fix side-effect in get_qual_for_list()

[Jeevan Ladhe]

On Mon, May 29, 2017 at 11:55 PM, Tom Lane  wrote:

> Jeevan Ladhe  writes:
> > I have rebased the patch on recent commit.
>
> Pushed with some further tweaking.
>

Thanks Tom for taking care of this.

Regards,
Jeevan Ladhe

Re: [HACKERS] fix side-effect in get_qual_for_list()

[Tom Lane]

Jeevan Ladhe  writes:
> I have rebased the patch on recent commit.

Pushed with some further tweaking.

regards, tom lane


-- 
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] fix side-effect in get_qual_for_list()

[Jeevan Ladhe]

Hi,

I have rebased the patch on recent commit.

With recent commits, some of the hunks in the v2 patch related to
castNode, are not needed.

PFA.

Regards,
Jeevan Ladhe

On Sat, May 27, 2017 at 1:16 AM, Jeevan Ladhe  wrote:

> Hi Ashutosh,
>
> Thanks for catching this. For now this isn't a problem since
>> generate_partition_qual() is crafting PartitionBoundInfo which it
>> doesn't use anywhere else. But if the function gets used where the
>> PartitionBoundSpec is being used somewhere else as well.
>
>
> Yes, this behavior currently does not affect adversely, but I think this
> function is quite useful for future enhancements and should be fixed.
>
> While you are
>> at it, can we use castNode() in place of
>> PartitionBoundSpec *spec = (PartitionBoundSpec *) bound; Or do you
>> think it should be done separately?
>>
>
> I have made this change at couple of places applicable.
>
> PFA.
>
> Regards,
> Jeevan Ladhe
>


fix_listdatums_get_qual_for_list_v3.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] fix side-effect in get_qual_for_list()

[Jeevan Ladhe]

Hi Ashutosh,

Thanks for catching this. For now this isn't a problem since
> generate_partition_qual() is crafting PartitionBoundInfo which it
> doesn't use anywhere else. But if the function gets used where the
> PartitionBoundSpec is being used somewhere else as well.


Yes, this behavior currently does not affect adversely, but I think this
function is quite useful for future enhancements and should be fixed.

While you are
> at it, can we use castNode() in place of
> PartitionBoundSpec *spec = (PartitionBoundSpec *) bound; Or do you
> think it should be done separately?
>

I have made this change at couple of places applicable.

PFA.

Regards,
Jeevan Ladhe


fix_listdatums_get_qual_for_list_v2.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] fix side-effect in get_qual_for_list()

[Ashutosh Bapat]

On Thu, May 25, 2017 at 3:12 PM, Jeevan Ladhe
 wrote:
> Hi,
>
> While working on one of the crash reported on default partition for list
> partitioning table[1] I found some strange behavior in get_qual_for_list()
> while
> I tried to call it from the new code I wrote for default partition.
>
> After debugging, I noticed that the function get_qual_for_list() is
> implicitly
> manipulating the (PartitionBoundSpec) spec->listdatums list. AFAICU, this
> manipulation is needed just to construct a list of datums to be passed to
> ArrayExpr, and this should be done without manipulating the original list.
> The function name is get_qual_for_list(), which implies that this function
> returns something and does not modify anything.
>
> I have made this change in attached patch, as I think this is useful for
> future
> developments, as there may be a need in future to call get_qual_for_list()
> from
> other places, and the caller might not expect that PartitionBoundSpec gets
> modified.
>

Thanks for catching this. For now this isn't a problem since
generate_partition_qual() is crafting PartitionBoundInfo which it
doesn't use anywhere else. But if the function gets used where the
PartitionBoundSpec is being used somewhere else as well. While you are
at it, can we use castNode() in place of
PartitionBoundSpec *spec = (PartitionBoundSpec *) bound; Or do you
think it should be done separately?

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database 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] fix side-effect in get_qual_for_list()

[Jeevan Ladhe]

Hi,

While working on one of the crash reported on default partition for list
partitioning table[1] I found some strange behavior in get_qual_for_list()
while
I tried to call it from the new code I wrote for default partition.

After debugging, I noticed that the function get_qual_for_list() is
implicitly
manipulating the (PartitionBoundSpec) spec->listdatums list. AFAICU, this
manipulation is needed just to construct a list of datums to be passed to
ArrayExpr, and this should be done without manipulating the original list.
The function name is get_qual_for_list(), which implies that this function
returns something and does not modify anything.

I have made this change in attached patch, as I think this is useful for
future
developments, as there may be a need in future to call get_qual_for_list()
from
other places, and the caller might not expect that PartitionBoundSpec gets
modified.

PFA.

[1] 
*https://www.postgresql.org/message-id/flat/CAOgcT0PLPge%3D5U6%3DGU5SnC3_8yutCbWWOiUva3Cw94M9zpbvgQ%40mail.gmail.com
*

Regards,
Jeevan Ladhe


fix_listdatums_get_qual_for_list.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