Horton

Thread

Commits

  1. Reorder tests in get_cheapest_path_for_pathkeys().

  2. Fix an assortment of improper usages of string functions

  1. A minor adjustment to get_cheapest_path_for_pathkeys

    Richard Guo <guofenglinux@gmail.com> — 2023-04-11T07:19:41Z

    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.

Thanks
Richard

  2. Re: A minor adjustment to get_cheapest_path_for_pathkeys

    Aleksander Alekseev <aleksander@timescale.com> — 2023-07-11T12:16:38Z

    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

  3. Re: A minor adjustment to get_cheapest_path_for_pathkeys

    Richard Guo <guofenglinux@gmail.com> — 2023-07-19T10:12:07Z

    On Tue, Jul 11, 2023 at 8:16 PM Aleksander Alekseev <
aleksander@timescale.com> wrote:

> 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.


Thanks for the review!


> 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.

Another change in this patch is to mention the requirement about being
parallel-safe in the comment.

  *   Find the cheapest path (according to the specified criterion) that
- *   satisfies the given pathkeys and parameterization.
+ *   satisfies the given pathkeys and parameterization, and is
parallel-safe
+ *   if required.

Maybe this is something that is worthwhile to do?

Thanks
Richard

  4. Re: A minor adjustment to get_cheapest_path_for_pathkeys

    Aleksander Alekseev <aleksander@timescale.com> — 2023-09-04T12:35:12Z

    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

  5. Re: A minor adjustment to get_cheapest_path_for_pathkeys

    Robert Haas <robertmhaas@gmail.com> — 2023-09-05T15:09:54Z

    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

  6. Re: A minor adjustment to get_cheapest_path_for_pathkeys

    Aleksander Alekseev <aleksander@timescale.com> — 2023-09-05T16:01:09Z

    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

  7. Re: A minor adjustment to get_cheapest_path_for_pathkeys

    Aleksander Alekseev <aleksander@timescale.com> — 2023-09-05T16:05:14Z

    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

  8. Re: A minor adjustment to get_cheapest_path_for_pathkeys

    Robert Haas <robertmhaas@gmail.com> — 2023-09-05T17:14:40Z

    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

  9. Re: A minor adjustment to get_cheapest_path_for_pathkeys

    Andy Fan <zhihui.fan1213@gmail.com> — 2023-09-06T06:45:06Z

    On Wed, Sep 6, 2023 at 4:06 AM Robert Haas <robertmhaas@gmail.com> wrote:

> 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
<https://github.com/postgres/postgres/commit/8b26769bc441fffa8aad31dddc484c2f4043d2c9>
.
- 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" ?

-- 
Best Regards
Andy Fan

  10. Re: A minor adjustment to get_cheapest_path_for_pathkeys

    Andy Fan <zhihui.fan1213@gmail.com> — 2023-09-06T06:46:08Z

    On Wed, Sep 6, 2023 at 2:45 PM Andy Fan <zhihui.fan1213@gmail.com>
wrote:uld have a different conversation...

>
> I like this consultation, so +1 from me :)
>

s/consultation/conclusion.


-- 
Best Regards
Andy Fan

  11. Re: A minor adjustment to get_cheapest_path_for_pathkeys

    Andy Fan <zhihui.fan1213@gmail.com> — 2023-09-06T07:13:39Z

    >
> 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.
>

I forget to mention that the past contribution of the author should be a
factor as well.  Like Richard has provided lots of performance improvement,
bug fix, code reviews, so I believe more attention from committers should
be a reasonable request.

-- 
Best Regards
Andy Fan

  12. Re: A minor adjustment to get_cheapest_path_for_pathkeys

    Robert Haas <robertmhaas@gmail.com> — 2023-09-06T12:50:30Z

    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

  13. Re: A minor adjustment to get_cheapest_path_for_pathkeys

    Andy Fan <zhihui.fan1213@gmail.com> — 2023-09-06T13:00:20Z

    On Wed, Sep 6, 2023 at 8:50 PM Robert Haas <robertmhaas@gmail.com> wrote:

> 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.
>

Agreed.


>
> > 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.
>

Fair enough.


> > 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.
>

Glad to know that.  I have marked myself as a reviewer and mark this entry
as "Ready for Committer".

https://commitfest.postgresql.org/44/4286/

-- 
Best Regards
Andy Fan

  14. Re: A minor adjustment to get_cheapest_path_for_pathkeys

    Richard Guo <guofenglinux@gmail.com> — 2023-09-07T02:21:31Z

    On Wed, Sep 6, 2023 at 8:50 PM Robert Haas <robertmhaas@gmail.com> wrote:

> On Wed, Sep 6, 2023 at 2:45 AM Andy Fan <zhihui.fan1213@gmail.com> wrote:
> > 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.


Fair point.  So I had a go at writing a commit message for this patch as
attached.  Thanks for all the reviews.

Thanks
Richard

  15. Re: A minor adjustment to get_cheapest_path_for_pathkeys

    Aleksander Alekseev <aleksander@timescale.com> — 2023-09-07T09:32:39Z

    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

  16. Re: A minor adjustment to get_cheapest_path_for_pathkeys

    Robert Haas <robertmhaas@gmail.com> — 2023-09-07T18:42:00Z

    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

  17. Re: A minor adjustment to get_cheapest_path_for_pathkeys

    Richard Guo <guofenglinux@gmail.com> — 2023-09-08T06:08:34Z

    On Fri, Sep 8, 2023 at 2:42 AM Robert Haas <robertmhaas@gmail.com> wrote:

> Committed.


Thanks for pushing it!

Thanks
Richard