Memory consumed by child SpecialJoinInfo in partitionwise join planning

On Thu, Jul 27, 2023 at 10:10 PM Ashutosh Bapat <
ashutosh.bapat.oss@gmail.com> wrote:

The attached patch (0002) fixes this issue by
1. declaring child SpecialJoinInfo as a local variable, thus
allocating memory on the stack instead of CurrentMemoryContext. The
memory on the stack is freed automatically.
2. Freeing the Relids in SpecialJoinInfo explicitly after
SpecialJoinInfo has been used.

It doesn't seem too expensive to translate SpecialJoinInfos, so I think
it's OK to construct and free the SpecialJoinInfo for a child join on
the fly. So the approach in 0002 looks reasonable to me. But there is
something that needs to be fixed in 0002.

+   bms_free(child_sjinfo->commute_above_l);
+   bms_free(child_sjinfo->commute_above_r);
+   bms_free(child_sjinfo->commute_below_l);
+   bms_free(child_sjinfo->commute_below_r);

These four members in SpecialJoinInfo only contain outer join relids.
They do not need to be translated. So they would reference the same
memory areas in child_sjinfo as in parent_sjinfo. We should not free
them, otherwise they would become dangling pointers in parent sjinfo.

Thanks
Richard

Hi Richard,

On Fri, Jul 28, 2023 at 2:03 PM Richard Guo <guofenglinux@gmail.com> wrote:

It doesn't seem too expensive to translate SpecialJoinInfos, so I think
it's OK to construct and free the SpecialJoinInfo for a child join on
the fly. So the approach in 0002 looks reasonable to me. But there is
something that needs to be fixed in 0002.

Thanks for the review. I am fine if going ahead with 0002 is enough.

+   bms_free(child_sjinfo->commute_above_l);
+   bms_free(child_sjinfo->commute_above_r);
+   bms_free(child_sjinfo->commute_below_l);
+   bms_free(child_sjinfo->commute_below_r);

These four members in SpecialJoinInfo only contain outer join relids.
They do not need to be translated. So they would reference the same
memory areas in child_sjinfo as in parent_sjinfo. We should not free
them, otherwise they would become dangling pointers in parent sjinfo.

Good catch. Saved my time. I would have caught this rather hard way
when running regression. Thanks a lot.

I think we should 1. add an assert to make sure that commute_above_*
do not require any transations i.e. they do not contain any parent
relids of the child rels. 2. Do not free those. 3. Add a comment about
keeping the build and free functions in sync. I will work on those
next.

--
Best Wishes,
Ashutosh Bapat

On Fri, Jul 28, 2023 at 7:28 PM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
wrote:

+   bms_free(child_sjinfo->commute_above_l);
+   bms_free(child_sjinfo->commute_above_r);
+   bms_free(child_sjinfo->commute_below_l);
+   bms_free(child_sjinfo->commute_below_r);

These four members in SpecialJoinInfo only contain outer join relids.
They do not need to be translated. So they would reference the same
memory areas in child_sjinfo as in parent_sjinfo. We should not free
them, otherwise they would become dangling pointers in parent sjinfo.

Attached patchset fixing those.
0001 - patch to report planning memory, with to explain.out regression
output fix. We may consider committing this as well.
0002 - with your comment addressed above.

I think we should 1. add an assert to make sure that commute_above_*
do not require any transations i.e. they do not contain any parent
relids of the child rels.

Looking at the definitions of commute_above and commute_below relids and OJ
relids, I don't think these assertions are necessary. So didn't add those.

2. Do not free those.

Done

3. Add a comment about
keeping the build and free functions in sync.

Done.

--
Best Wishes,
Ashutosh Bapat

On Fri, Aug 4, 2023 at 2:11 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

Attached patchset fixing those.
0001 - patch to report planning memory, with to explain.out regression output fix. We may consider committing this as well.
0002 - with your comment addressed above.

0003 - Added this patch for handling SpecialJoinInfos for inner joins.
These SpecialJoinInfos are created on the fly for parent joins. They
can be created on the fly for child joins as well without requiring
any translations. Thus they do not require any new memory. This patch
is intended to be merged into 0002 finally.

Added to the upcoming commitfest https://commitfest.postgresql.org/44/4500/

--
Best Wishes,
Ashutosh Bapat

Hi Ashutosh,

On Wed, Aug 16, 2023 at 2:28 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Fri, Aug 4, 2023 at 2:11 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

Attached patchset fixing those.
0001 - patch to report planning memory, with to explain.out regression output fix. We may consider committing this as well.
0002 - with your comment addressed above.

0003 - Added this patch for handling SpecialJoinInfos for inner joins.
These SpecialJoinInfos are created on the fly for parent joins. They
can be created on the fly for child joins as well without requiring
any translations. Thus they do not require any new memory. This patch
is intended to be merged into 0002 finally.

I read this thread and have been reading the latest patch. At first
glance, it seems quite straightforward to me. I agree with Richard
that pfree()'ing 4 bitmapsets may not be a lot of added overhead. I
will study the patch a bit more.

Just one comment on 0003:

+   /*
+    * Dummy SpecialJoinInfos do not have any translated fields and hence have
+    * nothing to free.
+    */
+   if (child_sjinfo->jointype == JOIN_INNER)
+       return;

Should this instead be Assert(child_sjinfo->jointype != JOIN_INNER)?

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

On Wed, Sep 20, 2023 at 5:24 PM Amit Langote <amitlangote09@gmail.com> wrote:

I read this thread and have been reading the latest patch. At first
glance, it seems quite straightforward to me. I agree with Richard
that pfree()'ing 4 bitmapsets may not be a lot of added overhead. I
will study the patch a bit more.

Thanks.

Just one comment on 0003:

+   /*
+    * Dummy SpecialJoinInfos do not have any translated fields and hence have
+    * nothing to free.
+    */
+   if (child_sjinfo->jointype == JOIN_INNER)
+       return;

Should this instead be Assert(child_sjinfo->jointype != JOIN_INNER)?

try_partitionwise_join() calls free_child_sjinfo_members()
unconditionally i.e. it will be called even SpecialJoinInfos of INNER
join. Above condition filters those out early. In fact if we convert
into an Assert, in a production build the rest of the code will free
Relids which are referenced somewhere else causing dangling pointers.

--
Best Wishes,
Ashutosh Bapat

On Wed, Sep 20, 2023 at 10:24 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Wed, Sep 20, 2023 at 5:24 PM Amit Langote <amitlangote09@gmail.com> wrote:

Just one comment on 0003:

+   /*
+    * Dummy SpecialJoinInfos do not have any translated fields and hence have
+    * nothing to free.
+    */
+   if (child_sjinfo->jointype == JOIN_INNER)
+       return;

Should this instead be Assert(child_sjinfo->jointype != JOIN_INNER)?

try_partitionwise_join() calls free_child_sjinfo_members()
unconditionally i.e. it will be called even SpecialJoinInfos of INNER
join. Above condition filters those out early. In fact if we convert
into an Assert, in a production build the rest of the code will free
Relids which are referenced somewhere else causing dangling pointers.

You're right. I hadn't realized that the parent SpecialJoinInfo
passed to try_partitionwise_join() might itself be a dummy. I guess I
should've read the whole thing before commenting.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

On Thu, Sep 21, 2023 at 6:37 AM Amit Langote <amitlangote09@gmail.com> wrote:

On Wed, Sep 20, 2023 at 10:24 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Wed, Sep 20, 2023 at 5:24 PM Amit Langote <amitlangote09@gmail.com> wrote:

Just one comment on 0003:

+   /*
+    * Dummy SpecialJoinInfos do not have any translated fields and hence have
+    * nothing to free.
+    */
+   if (child_sjinfo->jointype == JOIN_INNER)
+       return;

Should this instead be Assert(child_sjinfo->jointype != JOIN_INNER)?

try_partitionwise_join() calls free_child_sjinfo_members()
unconditionally i.e. it will be called even SpecialJoinInfos of INNER
join. Above condition filters those out early. In fact if we convert
into an Assert, in a production build the rest of the code will free
Relids which are referenced somewhere else causing dangling pointers.

You're right. I hadn't realized that the parent SpecialJoinInfo
passed to try_partitionwise_join() might itself be a dummy. I guess I
should've read the whole thing before commenting.

Maybe there's something to improve in terms of readability, I don't know.

--
Best Wishes,
Ashutosh Bapat

Hi Ashutosh,

On Thu, Sep 21, 2023 at 1:20 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Thu, Sep 21, 2023 at 6:37 AM Amit Langote <amitlangote09@gmail.com> wrote:

On Wed, Sep 20, 2023 at 10:24 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Wed, Sep 20, 2023 at 5:24 PM Amit Langote <amitlangote09@gmail.com> wrote:

Just one comment on 0003:

+   /*
+    * Dummy SpecialJoinInfos do not have any translated fields and hence have
+    * nothing to free.
+    */
+   if (child_sjinfo->jointype == JOIN_INNER)
+       return;

Should this instead be Assert(child_sjinfo->jointype != JOIN_INNER)?

try_partitionwise_join() calls free_child_sjinfo_members()
unconditionally i.e. it will be called even SpecialJoinInfos of INNER
join. Above condition filters those out early. In fact if we convert
into an Assert, in a production build the rest of the code will free
Relids which are referenced somewhere else causing dangling pointers.

You're right. I hadn't realized that the parent SpecialJoinInfo
passed to try_partitionwise_join() might itself be a dummy. I guess I
should've read the whole thing before commenting.

Maybe there's something to improve in terms of readability, I don't know.

Here are some comments.

Please merge 0003 into 0002.

+   /*
+    * But the list of operator OIDs and the list of expressions may be
+    * referenced somewhere else. Do not free those.
+    */

I don't see build_child_join_sjinfo() copying any list of OIDs, so I'm
not sure what the comment is referring to. Also, instead of "the list
of expressions", it might be more useful to mention semi_rhs_expr,
because that's the only one being copied.

The comment for SpecialJoinInfo mentions that they are stored in
PlannerInfo.join_info_list, but the child SpecialJoinInfos used by
partitionwise joins are not, which I noticed has not been mentioned
anywhere. Should we make a note of that in the SpecialJoinInfo's
comment?

Just out of curiosity, is their not being present in join_info_list
problematic in some manner, such as missed optimization opportunities
for child joins? I noticed there is a loop over join_info_list in
add_paths_to_join_rel(), which does get called for child joinrels. I
know this a bit off-topic for the thread, but thought to ask in case
you've already thought about it.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

On Wed, Sep 27, 2023 at 2:30 PM Amit Langote <amitlangote09@gmail.com> wrote:

Here are some comments.

Thanks for your review.

Please merge 0003 into 0002.

Done.

+   /*
+    * But the list of operator OIDs and the list of expressions may be
+    * referenced somewhere else. Do not free those.
+    */

I don't see build_child_join_sjinfo() copying any list of OIDs, so I'm
not sure what the comment is referring to. Also, instead of "the list
of expressions", it might be more useful to mention semi_rhs_expr,
because that's the only one being copied.

list of OID is semi_operators. It's copied as is from parent
SpecialJoinInfo. But the way it's mentioned in the comment is
confusing. I have modified the prologue of function to provide a
general guidance on what can be freed and what should not be. and then
specific examples. Let me know if this is more clear.

The comment for SpecialJoinInfo mentions that they are stored in
PlannerInfo.join_info_list, but the child SpecialJoinInfos used by
partitionwise joins are not, which I noticed has not been mentioned
anywhere. Should we make a note of that in the SpecialJoinInfo's
comment?

Good point. Since SpecialJoinInfos for JOIN_INNER are mentioned there,
it makes sense to mention transient child SpecialJoinInfos as well.
Done.

Just out of curiosity, is their not being present in join_info_list
problematic in some manner, such as missed optimization opportunities
for child joins? I noticed there is a loop over join_info_list in
add_paths_to_join_rel(), which does get called for child joinrels. I
know this a bit off-topic for the thread, but thought to ask in case
you've already thought about it.

The function has a comment and code to take care of this at the very beginning
/*
* PlannerInfo doesn't contain the SpecialJoinInfos created for joins
* between child relations, even if there is a SpecialJoinInfo node for
* the join between the topmost parents. So, while calculating Relids set
* representing the restriction, consider relids of topmost parent of
* partitions.
*/
if (joinrel->reloptkind == RELOPT_OTHER_JOINREL)
joinrelids = joinrel->top_parent_relids;
else
joinrelids = joinrel->relids;

PFA latest patch set
0001 - same as previous patch set
0002 - 0002 and 0003 from previous patch set squashed together
0003 - changes addressing above comments.

--
Best Wishes,
Ashutosh Bapat

On Wed, Sep 27, 2023 at 8:07 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Wed, Sep 27, 2023 at 2:30 PM Amit Langote <amitlangote09@gmail.com> wrote:

Just out of curiosity, is their not being present in join_info_list
problematic in some manner, such as missed optimization opportunities
for child joins? I noticed there is a loop over join_info_list in
add_paths_to_join_rel(), which does get called for child joinrels. I
know this a bit off-topic for the thread, but thought to ask in case
you've already thought about it.

The function has a comment and code to take care of this at the very beginning
/*
* PlannerInfo doesn't contain the SpecialJoinInfos created for joins
* between child relations, even if there is a SpecialJoinInfo node for
* the join between the topmost parents. So, while calculating Relids set
* representing the restriction, consider relids of topmost parent of
* partitions.
*/
if (joinrel->reloptkind == RELOPT_OTHER_JOINREL)
joinrelids = joinrel->top_parent_relids;
else
joinrelids = joinrel->relids;

Ah, that's accounted for. Thanks.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

On Wed, Sep 27, 2023 at 8:07 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Wed, Sep 27, 2023 at 2:30 PM Amit Langote <amitlangote09@gmail.com> wrote:

+   /*
+    * But the list of operator OIDs and the list of expressions may be
+    * referenced somewhere else. Do not free those.
+    */

I don't see build_child_join_sjinfo() copying any list of OIDs, so I'm
not sure what the comment is referring to. Also, instead of "the list
of expressions", it might be more useful to mention semi_rhs_expr,
because that's the only one being copied.

list of OID is semi_operators. It's copied as is from parent
SpecialJoinInfo. But the way it's mentioned in the comment is
confusing. I have modified the prologue of function to provide a
general guidance on what can be freed and what should not be. and then
specific examples. Let me know if this is more clear.

Thanks. I would've kept the notes about specific members inside the
function. Also, no need to have a note for pointers that are not
deep-copied to begin with, such as semi_operators. IOW, something
like the following would have sufficed:

@@ -1735,6 +1735,10 @@ build_child_join_sjinfo(PlannerInfo *root,
SpecialJoinInfo *parent_sjinfo,
 /*
  * free_child_sjinfo_members
  *     Free memory consumed by members of a child SpecialJoinInfo.
+ *
+ * Only members that are translated copies of their counterpart in the parent
+ * SpecialJoinInfo are freed here.  However, members that could be referenced
+ * elsewhere are not freed.
  */
 static void
 free_child_sjinfo_members(SpecialJoinInfo *child_sjinfo)
@@ -1755,10 +1759,7 @@ free_child_sjinfo_members(SpecialJoinInfo *child_sjinfo)
    bms_free(child_sjinfo->syn_lefthand);
    bms_free(child_sjinfo->syn_righthand);

-   /*
-    * But the list of operator OIDs and the list of expressions may be
-    * referenced somewhere else. Do not free those.
-    */
+   /* semi_rhs_exprs may be referenced, so don't free. */
 }

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

On Fri, Sep 29, 2023 at 8:36 AM Amit Langote <amitlangote09@gmail.com> wrote:

IOW, something
like the following would have sufficed:

@@ -1735,6 +1735,10 @@ build_child_join_sjinfo(PlannerInfo *root,
SpecialJoinInfo *parent_sjinfo,
/*
* free_child_sjinfo_members
*     Free memory consumed by members of a child SpecialJoinInfo.
+ *
+ * Only members that are translated copies of their counterpart in the parent
+ * SpecialJoinInfo are freed here.  However, members that could be referenced
+ * elsewhere are not freed.
*/
static void
free_child_sjinfo_members(SpecialJoinInfo *child_sjinfo)
@@ -1755,10 +1759,7 @@ free_child_sjinfo_members(SpecialJoinInfo *child_sjinfo)
bms_free(child_sjinfo->syn_lefthand);
bms_free(child_sjinfo->syn_righthand);

-   /*
-    * But the list of operator OIDs and the list of expressions may be
-    * referenced somewhere else. Do not free those.
-    */
+   /* semi_rhs_exprs may be referenced, so don't free. */
}

Works for me. PFA patchset with these changes. I have still left the
changes addressing your comments as a separate patch for easier
review.

--
Best Wishes,
Ashutosh Bapat

Rebased patchset. No changes in actual patches. 0001 is squashed
version of patches submitted at
/messages/by-id/CAExHW5sCJX7696sF-OnugAiaXS=Ag95=-m1cSrjcmyYj8Pduuw@mail.gmail.com.

On Tue, Oct 3, 2023 at 6:42 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Fri, Sep 29, 2023 at 8:36 AM Amit Langote <amitlangote09@gmail.com> wrote:

IOW, something
like the following would have sufficed:

@@ -1735,6 +1735,10 @@ build_child_join_sjinfo(PlannerInfo *root,
SpecialJoinInfo *parent_sjinfo,
/*
* free_child_sjinfo_members
*     Free memory consumed by members of a child SpecialJoinInfo.
+ *
+ * Only members that are translated copies of their counterpart in the parent
+ * SpecialJoinInfo are freed here.  However, members that could be referenced
+ * elsewhere are not freed.
*/
static void
free_child_sjinfo_members(SpecialJoinInfo *child_sjinfo)
@@ -1755,10 +1759,7 @@ free_child_sjinfo_members(SpecialJoinInfo *child_sjinfo)
bms_free(child_sjinfo->syn_lefthand);
bms_free(child_sjinfo->syn_righthand);

-   /*
-    * But the list of operator OIDs and the list of expressions may be
-    * referenced somewhere else. Do not free those.
-    */
+   /* semi_rhs_exprs may be referenced, so don't free. */
}

Works for me. PFA patchset with these changes. I have still left the
changes addressing your comments as a separate patch for easier
review.

--
Best Wishes,
Ashutosh Bapat

--
Best Wishes,
Ashutosh Bapat

On Wed, 4 Oct 2023 at 04:02, Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Fri, Sep 29, 2023 at 8:36 AM Amit Langote <amitlangote09@gmail.com> wrote:

IOW, something
like the following would have sufficed:

@@ -1735,6 +1735,10 @@ build_child_join_sjinfo(PlannerInfo *root,
SpecialJoinInfo *parent_sjinfo,
/*
* free_child_sjinfo_members
*     Free memory consumed by members of a child SpecialJoinInfo.
+ *
+ * Only members that are translated copies of their counterpart in the parent
+ * SpecialJoinInfo are freed here.  However, members that could be referenced
+ * elsewhere are not freed.
*/
static void
free_child_sjinfo_members(SpecialJoinInfo *child_sjinfo)
@@ -1755,10 +1759,7 @@ free_child_sjinfo_members(SpecialJoinInfo *child_sjinfo)
bms_free(child_sjinfo->syn_lefthand);
bms_free(child_sjinfo->syn_righthand);

-   /*
-    * But the list of operator OIDs and the list of expressions may be
-    * referenced somewhere else. Do not free those.
-    */
+   /* semi_rhs_exprs may be referenced, so don't free. */
}

Works for me. PFA patchset with these changes. I have still left the
changes addressing your comments as a separate patch for easier
review.

CFBot shows that the patch does not apply anymore as in [1]http://cfbot.cputube.org/patch_46_4500.log:

=== Applying patches on top of PostgreSQL commit ID
55627ba2d334ce98e1f5916354c46472d414bda6 ===
=== applying patch
./0001-Report-memory-used-for-planning-a-query-in--20231003.patch
...
Hunk #1 succeeded at 89 (offset -3 lines).
patching file src/test/regress/expected/explain.out
Hunk #5 FAILED at 285.
Hunk #6 succeeded at 540 (offset 4 lines).
1 out of 6 hunks FAILED -- saving rejects to file
src/test/regress/expected/explain.out.rej

Please post an updated version for the same.

[1]: http://cfbot.cputube.org/patch_46_4500.log

Regards,
Vignesh

On Fri, Jan 26, 2024 at 8:42 PM vignesh C <vignesh21@gmail.com> wrote:

On Wed, 4 Oct 2023 at 04:02, Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Fri, Sep 29, 2023 at 8:36 AM Amit Langote <amitlangote09@gmail.com> wrote:

IOW, something
like the following would have sufficed:

.. snip...

Works for me. PFA patchset with these changes. I have still left the
changes addressing your comments as a separate patch for easier
review.

CFBot shows that the patch does not apply anymore as in [1]:

=== Applying patches on top of PostgreSQL commit ID
55627ba2d334ce98e1f5916354c46472d414bda6 ===
=== applying patch
./0001-Report-memory-used-for-planning-a-query-in--20231003.patch
...
Hunk #1 succeeded at 89 (offset -3 lines).
patching file src/test/regress/expected/explain.out
Hunk #5 FAILED at 285.
Hunk #6 succeeded at 540 (offset 4 lines).
1 out of 6 hunks FAILED -- saving rejects to file
src/test/regress/expected/explain.out.rej

Please post an updated version for the same.

[1] - http://cfbot.cputube.org/patch_46_4500.log

Thanks Vignesh. PFA patches rebased on the latest HEAD. The patch
addressing Amit's comments is still a separate patch for him to
review.

--
Best Wishes,
Ashutosh Bapat

On 30/1/2024 12:44, Ashutosh Bapat wrote:

Thanks Vignesh. PFA patches rebased on the latest HEAD. The patch
addressing Amit's comments is still a separate patch for him to
review.

Thanks for this improvement. Working with partitions, I frequently see
peaks of memory consumption during planning. So, maybe one more case can
be resolved here.
Patch 0001 looks good. I'm not sure about free_child_sjinfo_members. Do
we really need it as a separate routine? It might be better to inline
this code.
Patch 0002 adds valuable comments, and I'm OK with that.

Also, as I remember, some extensions, such as pg_hint_plan, call
build_child_join_sjinfo. It is OK to break the interface with a major
version. But what if they need child_sjinfo a bit longer and collect
links to this structure? I don't think it is a real stopper, but it is
worth additional analysis.

--
regards,
Andrei Lepikhov
Postgres Professional

On Wed, Feb 14, 2024 at 9:50 AM Andrei Lepikhov
<a.lepikhov@postgrespro.ru> wrote:

On 30/1/2024 12:44, Ashutosh Bapat wrote:

Thanks Vignesh. PFA patches rebased on the latest HEAD. The patch
addressing Amit's comments is still a separate patch for him to
review.

Thanks for this improvement. Working with partitions, I frequently see
peaks of memory consumption during planning. So, maybe one more case can
be resolved here.
Patch 0001 looks good. I'm not sure about free_child_sjinfo_members. Do
we really need it as a separate routine? It might be better to inline
this code.

try_partitionwise_join() is already 200 lines long. A separate
function is better than adding more lines to try_partitionwise_join().
Also if someone wants to call build_child_join_sjinfo() outside
try_partitionwise_join() may find free_child_sjinfo_members() handy.

Patch 0002 adds valuable comments, and I'm OK with that.

Also, as I remember, some extensions, such as pg_hint_plan, call
build_child_join_sjinfo. It is OK to break the interface with a major
version. But what if they need child_sjinfo a bit longer and collect
links to this structure? I don't think it is a real stopper, but it is
worth additional analysis.

If these extensions call build_child_join_sjinfo() and do not call
free_child_sjinfo_members, they can keep their child sjinfo as long as
they want. I didn't understand your concern.

--
Best Wishes,
Ashutosh Bapat

On 14/2/2024 13:32, Ashutosh Bapat wrote:

On Wed, Feb 14, 2024 at 9:50 AM Andrei Lepikhov
<a.lepikhov@postgrespro.ru> wrote:

On 30/1/2024 12:44, Ashutosh Bapat wrote:

Thanks Vignesh. PFA patches rebased on the latest HEAD. The patch
addressing Amit's comments is still a separate patch for him to
review.

Thanks for this improvement. Working with partitions, I frequently see
peaks of memory consumption during planning. So, maybe one more case can
be resolved here.
Patch 0001 looks good. I'm not sure about free_child_sjinfo_members. Do
we really need it as a separate routine? It might be better to inline
this code.

try_partitionwise_join() is already 200 lines long. A separate
function is better than adding more lines to try_partitionwise_join().
Also if someone wants to call build_child_join_sjinfo() outside
try_partitionwise_join() may find free_child_sjinfo_members() handy.

Make sense, thanks.

Patch 0002 adds valuable comments, and I'm OK with that.

Also, as I remember, some extensions, such as pg_hint_plan, call
build_child_join_sjinfo. It is OK to break the interface with a major
version. But what if they need child_sjinfo a bit longer and collect
links to this structure? I don't think it is a real stopper, but it is
worth additional analysis.

If these extensions call build_child_join_sjinfo() and do not call
free_child_sjinfo_members, they can keep their child sjinfo as long as
they want. I didn't understand your concern.

Nothing special. This patch makes external code responsible for
allocation of the structure and it adds more paths to do something
wrong. Current code looks good.

--
regards,
Andrei Lepikhov
Postgres Professional