Thread: [HACKERS] remove unnecessary flag has_null from PartitionBoundInfoData
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.
Regards,
Jeevan Ladhe
Attachment
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
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.
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
Attachment
Re: [HACKERS] remove unnecessary flag has_null from PartitionBoundInfoData
From
Ashutosh Bapat
Date:
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