Remove some redundant set_cheapest() calls

Richard Guo <guofenglinux@gmail.com> writes:

I happened to notice that the set_cheapest() calls in functions
set_namedtuplestore_pathlist() and set_result_pathlist() are redundant,
as we've centralized the set_cheapest() calls in set_rel_pathlist().

Attached is a trivial patch to remove these calls.

Agreed, and pushed.

BTW, I suspect that the set_cheapest() call in set_dummy_rel_pathlist()
is also redundant. The comment there says "This is redundant when we're
called from set_rel_size(), but not when called from elsewhere". I
doubt it. The other places where it is called are set_append_rel_size()
and set_subquery_pathlist(), both being called from set_rel_size(). So
set_cheapest() would ultimately be called in set_rel_pathlist().

I'm less convinced about changing this. I'd rather keep it consistent
with mark_dummy_rel.

regards, tom lane

On Wed, Mar 27, 2024 at 4:06 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Richard Guo <guofenglinux@gmail.com> writes:

I happened to notice that the set_cheapest() calls in functions
set_namedtuplestore_pathlist() and set_result_pathlist() are redundant,
as we've centralized the set_cheapest() calls in set_rel_pathlist().

Attached is a trivial patch to remove these calls.

Agreed, and pushed.

Thanks for pushing!

BTW, I suspect that the set_cheapest() call in set_dummy_rel_pathlist()
is also redundant. The comment there says "This is redundant when we're
called from set_rel_size(), but not when called from elsewhere". I
doubt it. The other places where it is called are set_append_rel_size()
and set_subquery_pathlist(), both being called from set_rel_size(). So
set_cheapest() would ultimately be called in set_rel_pathlist().

I'm less convinced about changing this. I'd rather keep it consistent
with mark_dummy_rel.

Hm, I wonder if we should revise the comment there that states "but not
when called from elsewhere", as it does not seem to be true.

Thanks
Richard

Richard Guo <guofenglinux@gmail.com> writes:

On Wed, Mar 27, 2024 at 4:06 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'm less convinced about changing this. I'd rather keep it consistent
with mark_dummy_rel.

Hm, I wonder if we should revise the comment there that states "but not
when called from elsewhere", as it does not seem to be true.

I'd be okay with wording like "This is redundant in current usage
because set_rel_pathlist will do it later, but it's cheap so we keep
it for consistency with mark_dummy_rel". What do you think?

regards, tom lane

On Wed, Mar 27, 2024 at 10:59 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Richard Guo <guofenglinux@gmail.com> writes:

On Wed, Mar 27, 2024 at 4:06 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'm less convinced about changing this. I'd rather keep it consistent
with mark_dummy_rel.

Hm, I wonder if we should revise the comment there that states "but not
when called from elsewhere", as it does not seem to be true.

I'd be okay with wording like "This is redundant in current usage
because set_rel_pathlist will do it later, but it's cheap so we keep
it for consistency with mark_dummy_rel". What do you think?

That works for me. Thanks for the wording.

Thanks
Richard