Обсуждение: Remove some redundant set_cheapest() calls

Поиск
Список
Период
Сортировка

Remove some redundant set_cheapest() calls

От
Richard Guo
Дата:
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.

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

Thoughts?

Thanks
Richard
Вложения

Re: Remove some redundant set_cheapest() calls

От
Tom Lane
Дата:
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



Re: Remove some redundant set_cheapest() calls

От
Richard Guo
Дата:

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

Re: Remove some redundant set_cheapest() calls

От
Tom Lane
Дата:
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



Re: Remove some redundant set_cheapest() calls

От
Richard Guo
Дата:

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