minor change for create_list_bounds()
Hi,
I was looking at commit db632fbca and noticed that,
in create_list_bounds(), if index is added to boundinfo->interleaved_parts
in the first if statement, there is no need to perform the second check
involving call to partition_bound_accepts_nulls().
Here is a short patch.
Cheers
Attachments:
list-bound-accepts-null.patchapplication/octet-stream; name=list-bound-accepts-null.patchDownload
diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index 6b35111ebb..9cd072ee69 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -651,7 +651,7 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
* Mark the NULL partition as interleaved if we find that it
* allows some other non-NULL Datum.
*/
- if (partition_bound_accepts_nulls(boundinfo) &&
+ else if (partition_bound_accepts_nulls(boundinfo) &&
index == boundinfo->null_index)
boundinfo->interleaved_parts = bms_add_member(boundinfo->interleaved_parts,
boundinfo->null_index);
On Tue, Mar 08, 2022 at 11:05:10AM -0800, Zhihong Yu wrote:
I was looking at commit db632fbca and noticed that,
in create_list_bounds(), if index is added to boundinfo->interleaved_parts
in the first if statement, there is no need to perform the second check
involving call to partition_bound_accepts_nulls().
Given this change probably doesn't meaningfully impact performance or code
clarity, I'm personally -1 for this patch. Is there another motivation
that I am missing?
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Thu, 30 Jun 2022 at 11:41, Nathan Bossart <nathandbossart@gmail.com> wrote:
On Tue, Mar 08, 2022 at 11:05:10AM -0800, Zhihong Yu wrote:
I was looking at commit db632fbca and noticed that,
in create_list_bounds(), if index is added to boundinfo->interleaved_parts
in the first if statement, there is no need to perform the second check
involving call to partition_bound_accepts_nulls().Given this change probably doesn't meaningfully impact performance or code
clarity, I'm personally -1 for this patch. Is there another motivation
that I am missing?
While I agree that the gains on making this change are small. It just
accounts to saving a call to bms_add_member() when we've already found
the partition to be interleaved due to interleaved Datum values, I
just disagree with not doing anything about it. My reasons are:
1. This code is new to PG15. We have the opportunity now to make a
meaningful improvement and backpatch it. When PG15 is out, the bar is
set significantly higher for fixing this type of thing due to having
to consider the additional cost of backpatching conflicts with other
future fixes in that area.
2. I think the code as I just pushed it is easier to understand than
what was there before.
3. I'd like to encourage people to look at and critique our newly
added code. Having a concern addressed seems like a good reward for
the work.
I've now pushed the patch along with some other minor adjustments in the area.
Thanks for the report/patch.
David
On Wed, Jul 13, 2022 at 05:07:53PM +1200, David Rowley wrote:
While I agree that the gains on making this change are small. It just
accounts to saving a call to bms_add_member() when we've already found
the partition to be interleaved due to interleaved Datum values, I
just disagree with not doing anything about it. My reasons are:1. This code is new to PG15. We have the opportunity now to make a
meaningful improvement and backpatch it. When PG15 is out, the bar is
set significantly higher for fixing this type of thing due to having
to consider the additional cost of backpatching conflicts with other
future fixes in that area.
2. I think the code as I just pushed it is easier to understand than
what was there before.
Fair enough.
3. I'd like to encourage people to look at and critique our newly
added code. Having a concern addressed seems like a good reward for
the work.
+1
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com