Re: Reducing memory consumed by RestrictInfo list translations in partitionwise join planning

On Tue, Mar 25, 2025 at 3:17 PM Ashutosh Bapat
<ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
> On Mon, Mar 24, 2025 at 7:23 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> >
> > Ok, thanks for that analysis. I don't think there's anything about
> > the patch that makes it particularly less suitable for pwj=on.
> >
>
> I agree.
>
> > I read patch 0002 in detail last week and wrote a follow-up patch
> > (0003), mostly for cosmetic improvements, which I plan to squash into
> > 0002.
>
> For some reason v2-0003 didn't apply cleanly on my local branch.
> Possibly v2-0002 is a modified version of my 0002.

Ah, right -- I probably tweaked your 0002 a bit before splitting out
my changes into a separate patch.

> In order to not
> disturb your patchset, I have attached my edits as a diff. If you find
> those useful, apply them to 0003 and then squash it into 0002.
>
> Here are some more cosmetic comments on 0003. A bit of discussion
> might be needed. Hence did not edit myself.
>
> -/* Hash table entry in ec_derives_hash. */
> +/* Hash table entry used in ec_derives_hash. */
>
> I think we don't need "used" here entry in hash table is good enough. But I am
> ok even if it stays that way.

Ok, let's drop "used".

> - add_derived_clauses(ec1, ec2->ec_derives_list);
> + /* Updates ec1's ec_derives_list and ec_derives_hash if present. */
> + ec_add_derived_clauses(ec1, ec2->ec_derives_list);
>
> Suggestion in the attached diff.

Makes sense, though I added it after a small wording tweak to avoid
implying that the hash tables are merged in some special way. So now
it reads:

- /* Updates ec1's ec_derives_list and ec_derives_hash if present. */
+ /*
+ * Appends ec2's derived clauses to ec1->ec_derives_list and adds
+ * them to ec1->ec_derives_hash if present.
+ */

> @@ -396,7 +400,7 @@ process_equivalence(PlannerInfo *root,
> /* just to avoid debugging confusion w/ dangling pointers: */
> ec2->ec_members = NIL;
> ec2->ec_sources = NIL;
> - clear_ec_derived_clauses(ec2);
> + ec_clear_derived_clauses(ec2);
>
> I pondered about this naming convention when naming the functions. But
> it seems it's not used everywhere in this file OR I am not able to see
> the underlying naming rule if any. So I used a mixed naming. Let's
> keep your names though. I think they are better.

Got it -- I went with the ec_ prefix mainly to make the new additions
self-consistent, since the file doesn’t seem to follow a strict naming
pattern. Glad the names work for you. While at it, I also applied the
same naming convention to two new functions I hadn’t touched earlier
for some reason.

> @@ -1403,6 +1403,17 @@ typedef struct JoinDomain
> * entry: consider SELECT random() AS a, random() AS b ... ORDER BY b,a.
> * So we record the SortGroupRef of the originating sort clause.
> *
> + * Derived equality clauses between EC members are stored in ec_derives_list.
>
> "clauses between EC members" doesn't read correctly - "clauses
> equating EC members" seems better. We actually don't need to mention
> "between EC members" at all - it's not relevant to the "size of the
> list" which is the subject of this paragraph. What do you think?

OK, I agree -- we don't need to mention EC members here. I've updated
the comment to keep the focus on the list itself

> + * For small queries, this list is scanned directly during lookup. For larger
> + * queries -- e.g., with many partitions or joins -- a hash table
> + * (ec_derives_hash) is built for faster lookup. Both structures contain the
> + * same RestrictInfos and are maintained in parallel.
>
> If only the list exists, there is nothing to maintain in parallel. The
> sentence seems to indicate that both the hash table and the list are
> always present (and maintained in parallel). How about dropping "Both
> ... parallel" and modifying the previous sentence as " ... faster
> lookup when the list grows beyond a threshold" or something similar.
> The next sentence anyway mentions that both the structures are
> maintained.

Agree here too. Here's the revised comment addressing these two points:

+ * Derived equality clauses are stored in ec_derives_list. For small queries,
+ * this list is scanned directly during lookup. For larger queries -- e.g.,
+ * with many partitions or joins -- a hash table (ec_derives_hash) is built
+ * when the list grows beyond a threshold, for faster lookup. When present,
+ * the hash table contains the same RestrictInfos and is maintained alongside
+ * the list. We retain the list even when the hash is used to simplify
+ * serialization (e.g., in _outEquivalenceClass()) and support
+ * EquivalenceClass merging.

I've merged my delta + your suggested changes as discussed above into 0002.

Btw, about ec_clear_derived_clauses():

@@ -749,7 +749,7 @@ remove_rel_from_eclass(EquivalenceClass *ec,
SpecialJoinInfo *sjinfo,
* drop them. (At this point, any such clauses would be base restriction
* clauses, which we'd not need anymore anyway.)
*/
- ec->ec_derives = NIL;
+ ec_clear_derived_clauses(ec);
}

/*
@@ -1544,8 +1544,7 @@ update_eclasses(EquivalenceClass *ec, int from, int to)
list_free(ec->ec_members);
ec->ec_members = new_members;

- list_free(ec->ec_derives);
- ec->ec_derives = NULL;
+ ec_clear_derived_clauses(ec);

We're losing that list_free() in the second hunk, aren't we?

There's also this comment:

+ * XXX: When thousands of partitions are involved, the list can become
+ * sizable. It might be worth freeing it explicitly in such cases.

So maybe ec_clear_derived_clauses() should take a free_list parameter,
to preserve the original behavior? What do you think?

--
Thanks, Amit Langote