Re: [HACKERS] remove unnecessary flag has_null from PartitionBoundInfoData

2017-06-13 Thread Robert Haas 
On Mon, Jun 12, 2017 at 9:03 AM, Ashutosh Bapat
 wrote:
> On Mon, Jun 12, 2017 at 3:50 PM, amul sul  wrote:
>> On Wed, May 17, 2017 at 10:22 PM, Robert Haas  wrote:
>> [...]
>>> I committed this with fixes for those issues, plus I renamed the macro
>>> to partition_bound_accepts_nulls, which I think is more clear.
>>>
>> partition_bound_accepts_nulls() will alway yield true for a range
>> partitioning case, because in RelationBuildPartitionDesc, we forgot to
>> set boundinfo->null_index to -1.
>>
>> The attached patch fixes that.
>>
>
> Right now, the partition_bound_accepts_nulls() has two callers viz.
> check_new_partition_bound() and get_partition_for_tuple(). Both of
> those callers are calling it only in case of LIST partition. So,
> having null_index uninitialized in PartitionBoundInfoData is not a
> problem. But in general, we shouldn't leave a field uninitialized in
> that structure, so +1 for the patch.

I agree - that's a bug waiting to happen.  Committed.

-- 
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] remove unnecessary flag has_null from PartitionBoundInfoData

2017-06-12 Thread Ashutosh Bapat 
On Mon, Jun 12, 2017 at 3:50 PM, amul sul  wrote:
> On Wed, May 17, 2017 at 10:22 PM, Robert Haas  wrote:
> [...]
>> I committed this with fixes for those issues, plus I renamed the macro
>> to partition_bound_accepts_nulls, which I think is more clear.
>>
> partition_bound_accepts_nulls() will alway yield true for a range
> partitioning case, because in RelationBuildPartitionDesc, we forgot to
> set boundinfo->null_index to -1.
>
> The attached patch fixes that.
>

Right now, the partition_bound_accepts_nulls() has two callers viz.
check_new_partition_bound() and get_partition_for_tuple(). Both of
those callers are calling it only in case of LIST partition. So,
having null_index uninitialized in PartitionBoundInfoData is not a
problem. But in general, we shouldn't leave a field uninitialized in
that structure, so +1 for the patch.


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

Re: [HACKERS] remove unnecessary flag has_null from PartitionBoundInfoData

2017-06-12 Thread amul sul 
On Wed, May 17, 2017 at 10:22 PM, Robert Haas  wrote:
[...]
> I committed this with fixes for those issues, plus I renamed the macro
> to partition_bound_accepts_nulls, which I think is more clear.
>
partition_bound_accepts_nulls() will alway yield true for a range
partitioning case, because in RelationBuildPartitionDesc, we forgot to
set boundinfo->null_index to -1.

The attached patch fixes that.


Regards,
Amul


set_boundinfo-null_index.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] remove unnecessary flag has_null from PartitionBoundInfoData

2017-05-17 Thread Jeevan Ladhe 
> I committed this with fixes for those issues, plus I renamed the macro
> to partition_bound_accepts_nulls, which I think is more clear.


Thanks Robert.

Re: [HACKERS] remove unnecessary flag has_null from PartitionBoundInfoData

2017-05-17 Thread Robert Haas 
On Wed, May 17, 2017 at 6:53 AM, Jeevan Ladhe
 wrote:
> As discussed in default partition thread[1], here is the patch to remove
> has_null from PartitionBoundInfoData structure.
> Basically flag has_null is not needed and null_index can be checked if the
> current bound is having a null value or not.
>
> For simplicity of future use, in attached patch I have introduced a macro
> that
> would return TRUE if the given bound has null.

This seems like a good cleanup, but:

- It makes no sense to put a macro definition in a header file when
the corresponding structure definition is private to a specific .c
file.

- You failed to update the header comment for PartitionBoundInfoData
which mentions the has_null flag.

- The comment for null_index also seems like it should be updated to
mention that -1 is the sentinel value in general, not just for
range-partitioned tables.

I committed this with fixes for those issues, plus I renamed the macro
to partition_bound_accepts_nulls, which I think is more clear.

-- 
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] remove unnecessary flag has_null from PartitionBoundInfoData

2017-05-17 Thread Jeevan Ladhe 
Hi,

As discussed in default partition thread[1]
,
here is the patch to remove
has_null from PartitionBoundInfoData structure.
Basically flag has_null is not needed and null_index can be checked if the
current bound is having a null value or not.

For simplicity of future use, in attached patch I have introduced a macro
that
would return TRUE if the given bound has null.

PFA.

[1].
https://www.postgresql.org/message-id/CAFjFpRcvD-jf6z6-5mjx4tm-8NSj8Wnh%2B6%3DL6OB9Qy2yKX0HCg%40mail.gmail.com

Regards,
Jeevan Ladhe


remove_has_null_PartitionBoundInfoData_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