Re: [PoC] Reducing planning time when tables have many partitions

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