Re: A minor adjustment to get_cheapest_path_for_pathkeys

On Tue, Jul 11, 2023 at 8:16 PM Aleksander Alekseev <
aleksander(at)timescale(dot)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