Horton

Thread

Commits

  1. Always initialize PartitionBoundInfoData's null_index.

  2. Remove redundant has_null member from PartitionBoundInfoData.

  1. remove unnecessary flag has_null from PartitionBoundInfoData

    Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2017-05-17T10:53:50Z

    Hi,

As discussed in default partition thread[1]
<https://www.postgresql.org/message-id/CAFjFpRcvD-jf6z6-5mjx4tm-8NSj8Wnh%2B6%3DL6OB9Qy2yKX0HCg%40mail.gmail.com>,
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

  2. Re: remove unnecessary flag has_null from PartitionBoundInfoData

    Robert Haas <robertmhaas@gmail.com> — 2017-05-17T16:52:29Z

    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

  3. Re: remove unnecessary flag has_null from PartitionBoundInfoData

    Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2017-05-17T17:58:39Z

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

  4. Re: remove unnecessary flag has_null from PartitionBoundInfoData

    Amul Sul <sulamul@gmail.com> — 2017-06-12T10:20:49Z

    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

  5. Re: remove unnecessary flag has_null from PartitionBoundInfoData

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-06-12T13:03:00Z

    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

  6. Re: remove unnecessary flag has_null from PartitionBoundInfoData

    Robert Haas <robertmhaas@gmail.com> — 2017-06-13T16:41:19Z

    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