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...
I like this consultation, so +1 from me :)
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.
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.
- Authors can provide more refactor code if possible. like
8b26769bc.
- After others reviewers read the patch and think it is good to commit
with the rule above, who can mark the commitfest entry as "Ready
for Committer". Whenever a committer wants some non mental stress,
they can pick this and commit this.
Actually I also want to know what "Ready for Committer" is designed for,
and when/who can mark a patch as "Ready for Committer" ?