Re: Memory consumed by paths during partitionwise join planning

On Tue, Jul 22, 2025 at 7:31 PM Andrei Lepikhov <lepihov(at)gmail(dot)com> wrote:
>
> On 18/7/2025 13:48, Ashutosh Bapat wrote:
> > On Mon, Jul 7, 2025 at 8:43 PM Andrei Lepikhov <lepihov(at)gmail(dot)com> wrote:
> > if (!IsA(new_path, IndexPath))
> > - pfree(new_path);
> > + free_path(new_path, 0, false);
> >
> > Why don't we free the subpaths if they aren't referenced anymore?
> During testing, I discovered that we sometimes encounter unusual cases.
> For example, imagine pathlist:
> {SeqScan, IndexOnlyScan1, IndexScan2}
> Someone may decide that Sort+SeqScan may be cheaper than IndexScan2. Or,
> IncrementalSort+IndexOnlyScan1 is cheaper than IndexScan2 ... And add
> this path to the same pathlist. I am unsure which exact combinations may
> arise in the planner, but without strict rules, someone may forget to
> increment the refcounter.

When sort + seqscan path is created, seqscan's refcount will be
incremented, and if that dominates indexScan2 which in turn is removed
by add_path from pathlist, indexScan2 should be freed if no other path
is referencing it. One of the objectives of this patch is to make it
easy to maintain refcount. That's why the refcount is incremented in a
central place like path creation time or when adding to pathlist. The
case where it is not incremented should be studied and fixed.

>
> >> To address this complexity, I propose the following solutions:
> >> 1. Localise reference management within the add_path function.
> >> 2. Transition from a 'strict' approach, which removes all unused paths,
> >> to a more straightforward 'pinning' method. In this approach, we would
> >> simply mark a path as 'used' when someone who was added to the upper
> >> path list references it. Removing less memory, we leave the code much
> >> simpler.
> > Yes. This was one of the ideas Tom had proposed earlier in another
> > thread to manage paths better and avoid dangling pointers. May be it's
> > worth starting with that first. Get rid of special handling of index
> > paths and then improve the memory situation. However, even with that,
> > I think we should free more paths than less.
> It seems like one more trade-off: more eager cleaning means more
> resources spent.

Sorry, I wasn't clear. Even when using simple method to keep track of
whether a path has been referenced or not, we should be able to get
rid of special handling of IndexPath in add_path*
/* Reject and recycle the new path */
if (!IsA(new_path, IndexPath))
pfree(new_path);

That way, we will be able to free even the unreferenced index paths,
thus saving more memory than now.

>
> P.S. path_walker makes sense to implement.

+1

--
Best Wishes,
Ashutosh Bapat