Обсуждение: [HACKERS] remove unnecessary flag has_null from PartitionBoundInfoData

On Wed, May 17, 2017 at 6:53 AM, Jeevan Ladhe
<jeevan.ladhe@enterprisedb.com> 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

On Wed, May 17, 2017 at 10:22 PM, Robert Haas <robertmhaas@gmail.com> 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

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

On Mon, Jun 12, 2017 at 3:50 PM, amul sul <sulamul@gmail.com> wrote:
> On Wed, May 17, 2017 at 10:22 PM, Robert Haas <robertmhaas@gmail.com> 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

On Mon, Jun 12, 2017 at 9:03 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> On Mon, Jun 12, 2017 at 3:50 PM, amul sul <sulamul@gmail.com> wrote:
>> On Wed, May 17, 2017 at 10:22 PM, Robert Haas <robertmhaas@gmail.com> 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