Обсуждение: A minor adjustment to get_cheapest_path_for_pathkeys

Hi,

> The check for parallel_safe should be even cheaper than cost comparison
> so I think it's better to do that first.  The attached patch does this
> and also updates the comment to mention the requirement about being
> parallel-safe.

The patch was marked as "Needs review" so I decided to take a look.

I see the reasoning behind the proposed change, but I'm not convinced
that there will be any measurable performance improvements. Firstly,
compare_path_costs() is rather cheap. Secondly, require_parallel_safe
is `false` in most of the cases. Last but not least, one should prove
that this particular place is a bottleneck under given loads. I doubt
it is. Most of the time it's a network, disk I/O or locks.

So unless the author can provide benchmarks that show measurable
benefits of the change I suggest rejecting it.

-- 
Best regards,
Aleksander Alekseev

Hi,

>> I see the reasoning behind the proposed change, but I'm not convinced
>> that there will be any measurable performance improvements. Firstly,
>> compare_path_costs() is rather cheap. Secondly, require_parallel_safe
>> is `false` in most of the cases. Last but not least, one should prove
>> that this particular place is a bottleneck under given loads. I doubt
>> it is. Most of the time it's a network, disk I/O or locks.
>>
>> So unless the author can provide benchmarks that show measurable
>> benefits of the change I suggest rejecting it.
>
> Hmm, I doubt that there would be any measurable performance gains from
> this minor tweak.  I think this tweak is more about being cosmetic.  But
> I'm OK if it is deemed unnecessary and thus rejected.

During the triage of the patches submitted for the September CF a
consensus was reached [1] to mark this patch as Rejected.

[1]: https://postgr.es/m/0737f444-59bb-ac1d-2753-873c40da0840%40eisentraut.org

-- 
Best regards,
Aleksander Alekseev

On Mon, Sep 4, 2023 at 10:17 AM Aleksander Alekseev
<aleksander@timescale.com> wrote:
> During the triage of the patches submitted for the September CF a
> consensus was reached [1] to mark this patch as Rejected.

I don't think that's the correct conclusion. You said here that you
didn't think the patch was valuable. Then you said the same thing over
there. You agreeing with yourself is not a consensus.

I think this patch is a good idea and should be committed.

--
Robert Haas
EDB: http://www.enterprisedb.com

Hi Robert,

> I don't think that's the correct conclusion. You said here that you
> didn't think the patch was valuable. Then you said the same thing over
> there. You agreeing with yourself is not a consensus.

The word "consensus" was a poor choice for sure. The fact that I
suggested to reject the patch and nobody objected straight away is not
a consensus.

> I think this patch is a good idea and should be committed.

No problem, changing status back to "Needs review".

-- 
Best regards,
Aleksander Alekseev

Hi,

> > I think this patch is a good idea and should be committed.
>
> No problem, changing status back to "Needs review".

Now when we continue reviewing the patch, could you please elaborate a
bit on why you think it's worth committing?

-- 
Best regards,
Aleksander Alekseev

On Tue, Sep 5, 2023 at 12:05 PM Aleksander Alekseev
<aleksander@timescale.com> wrote:
> Now when we continue reviewing the patch, could you please elaborate a
> bit on why you think it's worth committing?

Well, why not? The test he's proposing to move earlier doesn't involve
calling a function, so it should be cheaper than the one he's moving
it past, which does.

I mean, I don't know whether the savings are going to be measurable on
a benchmark, but I guess I don't particularly see why it matters. Why
write a function that says "this thing is cheaper so we test it first"
and then perform a cheaper test afterwards? That's just silly. We can
either change the comment to say "we do this first for no reason even
though it would be more sensible to do the cheap test first" or we can
reorder the tests to match the principle set forth in the existing
comment.

I mean, unless there's some reason why it *isn't* cheaper. In that
case we should have a different conversation...

--
Robert Haas
EDB: http://www.enterprisedb.com

On Wed, Sep 6, 2023 at 2:45 AM Andy Fan <zhihui.fan1213@gmail.com> wrote:
> I guess the *valuable* sometimes means the effort we pay is greater
> than the benefit we get,  As for this patch,  the benefit is not huge (it
> is possible the compiler already does that). and the most effort we pay
> should be committer's attention, who needs to grab the patch, write the
> correct  commit and credit to the author and push it.  I'm not sure if
> Aleksander is worrying that this kind of patch will grab too much of
> the committer's attention and I do see there are lots of patches like
> this.

Very fair point. However, as you said in your follow-up email, Richard
Guo has done a lot of good work in this area already, so it makes
sense to pay a bit more attention to his suggestions.

> In my opinion,  we can do some stuff to improve the ROI.
> -  Authors should do as much as possible,  mainly a better commit
> message.  As for this patch, the commit message is " Adjustment
> to get_cheapest_path_for_pathkeys"  which I don't think matches
> our culture.

I agree. I don't think the patch submitter is obliged to try to write
a good commit message, but people who contribute regularly or are
posting large stacks of complex patches are probably well-advised to
try. It makes life easier for committers and even for reviewers trying
to make sense of their patches.

> Actually I also want to know what "Ready for Committer" is designed for,
> and when/who can mark a patch as "Ready for Committer" ?

Any reviewer who feels that this is the case. It's not binding on
anyone; it's an opinion.

--
Robert Haas
EDB: http://www.enterprisedb.com

Hi,

>> I agree. I don't think the patch submitter is obliged to try to write
>> a good commit message, but people who contribute regularly or are
>> posting large stacks of complex patches are probably well-advised to
>> try. It makes life easier for committers and even for reviewers trying
>> to make sense of their patches.
>
>
> Fair point.  So I had a go at writing a commit message for this patch as
> attached.  Thanks for all the reviews.

+1 to Robert's and Andy's arguments above. IMO the problem with the
patch was that it was declared as a performance improvement. In such
cases we typically ask the authors to prove that the actual
improvement took place and that there were no degradations.

If we consider the patch marley as a refactoring that improves the
readability I see no reason not to merge it.

v2 LGTM.

-- 
Best regards,
Aleksander Alekseev

On Thu, Sep 7, 2023 at 5:32 AM Aleksander Alekseev
<aleksander@timescale.com> wrote:
> v2 LGTM.

Committed.

--
Robert Haas
EDB: http://www.enterprisedb.com