On 7/8/2023 15:19, Yuya Watari wrote:
> Hello,
>
> Thank you for your reply.
>
> On Thu, Aug 3, 2023 at 10:29 PM Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
>> If you think that the verification is important to catch bugs, you may want to encapsulate it with an #ifdef ..
#endifsuch that the block within is not compiled by default. See OPTIMIZER_DEBUG for example.
>
> In my opinion, verifying the iteration results is only necessary to
> avoid introducing bugs while developing this patch. The verification
> is too excessive for regular development of PostgreSQL. I agree that
> we should avoid a significant degradation in assert enabled builds, so
> I will consider removing it.
I should admit, these checks has helped me during backpatching this
feature to pg v.13 (users crave speed up of query planning a lot). Maybe
it is a sign of a lack of tests, but in-fact, it already has helped.
One more thing: I think, you should add comments to
add_child_rel_equivalences() and add_child_join_rel_equivalences()
on replacing of:
if (bms_is_subset(cur_em->em_relids, top_parent_relids) &&
!bms_is_empty(cur_em->em_relids))
and
if (bms_overlap(cur_em->em_relids, top_parent_relids))
with different logic. What was changed? It will be better to help future
developers realize this part of the code more easily by adding some
comments.
>
>> Do you think that the memory measurement patch I have shared in those threads is useful in itself? If so, I will
startanother proposal to address it.
>
> For me, who is developing the planner in this thread, the memory
> measurement patch is useful. However, most users do not care about
> memory usage, so there is room for consideration. For example, making
> the metrics optional in EXPLAIN ANALYZE outputs might be better.
>
+1. Any memory-related info in the output of EXPLAIN ANALYZE makes tests
more complex because of architecture dependency.
--
regards,
Andrey Lepikhov
Postgres Professional