Thread: 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
Attachment
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