Обсуждение: minor change for create_list_bounds()

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