On Mon, Jul 7, 2025 at 8:43 PM Andrei Lepikhov <lepi...@gmail.com> wrote:
>
> On 27/6/2025 12:01, Andrei Lepikhov wrote:
> > On 6/2/2024 13:51, Ashutosh Bapat wrote:
> >> On Fri, Dec 15, 2023 at 5:22 AM Ashutosh Bapat
> >> <ashutosh.bapat....@gmail.com> wrote:
> >> First patch is no longer required. Here's rebased set
> >>
> >> The patches are raw. make check has some crashes that I need to fix. I
> >> am waiting to hear whether this is useful and whether the design is on
> >> the right track.
> > I think this project is quite important to move forward and discover how
> > far we can go this way. In the attachment, the rebased patch set with
> > fixes allows it to pass the regression tests.
+ case T_Group:
+ {
+ GroupPath *gpath = (GroupPath *) path;
+
+ unlink_path(&gpath->subpath, level, recurse, true);
+ }
+ break;
Thanks for adding those path types. Should we just create a path
walker routine like expression_walker?
That might be one reason why all the regression tests pass, but
there's another. We aren't freeing as many paths are we used to
/*
- * Delete the data pointed-to by the deleted cell, if possible
+ * TODO: write a routine to unlink a path from the list node and
+ * delete the list node
*/
- if (!IsA(old_path, IndexPath))
- pfree(old_path);
+ unlink_path(&old_path, 0, false, false);
If need_free = false, unlink_path will not free the path. Without this change we
were freeing at least the non-index paths but with this change, we aren't
freeing even those? Why so? In all the add_path variants we are doing this. So
effectively we are not freeing any paths that do not appear in the
rel->pathlist. So probably the planner is consuming more memory than earlier.
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?
> >
> > This idea of a refcounter resolves the problem with blind usage of
> > add_path. It is not only about extensions, which sometimes want to add
> > paths on different levels of the join tree and don't care about dangling
> > pointers. It is also about possible hidden bugs (a freed path staying in
> > the path list, just not employed) that aren't yet detected due to
> > costings and low test coverage.
Yeah. I started this project with a goal of reducing memory consumed
by planner when there are many partitions involved. But I think it
will be more useful for managing paths.
> After further consideration, I believe the main issue lies in managing
> increments and decrements of a path refcounter, especially in light of
> ongoing code changes. Additionally, recursion presents another challenge.
>
> 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.
--
Best Wishes,
Ashutosh Bapat
