Обсуждение: list_free in addRangeTableEntryForJoin

Hi Hackers

I have identified a potential memory leak in the 
`addRangeTableEntryForJoin()` function. The second parameter of 
`addRangeTableEntryForJoin()`, `colnames`, is a `List*` that is 
concatenated with another `List*`, `eref->colnames`, using 
`list_concat()`. We need to pass only the last `numaliases` elements of 
the list, for which we use `list_copy_tail`. This function creates a 
copy of the `colnames` list, resulting in `colnames` pointing to the 
current list that will not be freed. Consequently, a new list is already 
concatenated.

To address this issue, I have invoked `list_free(colnames)` afterwards. 
If anyone is aware of where the list is being freed or has any 
suggestions for improvement, I would greatly appreciate your input.

Best Regards,

Ilia Evdokimov,

TantorLabs LCC

 >But callers of addRangeTableEntryForJoin(), expects to handle a list 
or NIL, if we free the memory
I've thoroughly reviewed all callers of the 
`addRangeTableEntryForJoin()` function and confirmed that the list is 
not used after this function is called. Since 
`addRangeTableEntryForJoin()` is the last function to use this list, it 
would be more efficient to free the `List` at the point of its declaration.

I'll attach new patch where I free the lists.

Regards,

Ilia Evdokimov,

Tantor Labs LCC

 >Now you need to analyze whether the memory in question is not managed 
by a Context
I've already analyzed. Let's explain details:


1. analyze.c
1718: List* targetnames;
1815: targetnames = NIL;
1848: targetnames = lappend(targetnames, makeString(colName));
1871: addRangeTableEntryForJoin(...);
=> list_free(targetnames);

2. parse_clause.c
1163: List* res_colnames;
1289: res_colnames = NIL;
1324: foreach(col, res_colnames);
1396: res_colnames = lappend(res_colnames, lfirst(ucol));
1520, 1525: extractRemainingColumns(...);
        |
     290: *res_colnames = lappend(*res_colnames, lfirst(lc));
1543: addRangeTableEntryForJoin(...);
=> list_free(res_colnames);


As you can see, there are no other pointers working with this block of 
memory, and all operations above are either read-only or append nodes to 
the lists. If I am mistaken, please correct me.
Furthermore, I will call `list_free` immediately after 
`addRangeTableEntryForJoin()`. The new patch is attached.

Thanks for reviewing. I'm looking forward to new suggestions.

Regards,

Ilia Evdokimov,

Tantor Labs LCC

Ilia Evdokimov <ilya.evdokimov@tantorlabs.com> writes:
> I have identified a potential memory leak in the 
> `addRangeTableEntryForJoin()` function. The second parameter of 
> `addRangeTableEntryForJoin()`, `colnames`, is a `List*` that is 
> concatenated with another `List*`, `eref->colnames`, using 
> `list_concat()`. We need to pass only the last `numaliases` elements of 
> the list, for which we use `list_copy_tail`. This function creates a 
> copy of the `colnames` list, resulting in `colnames` pointing to the 
> current list that will not be freed. Consequently, a new list is already 
> concatenated.

> To address this issue, I have invoked `list_free(colnames)` afterwards. 

Sadly, I think this is basically a waste of development effort.
The parser, like the planner and rewriter and many other Postgres
subsystems, leaks tons of small allocations like this.  That's
*by design*, and *it's okay*, because we run these steps in short-
lived memory contexts that will be reclaimed in toto as soon as
the useful output data structures are no longer needed.  It's not
worth the sort of intellectual effort you've put in in this thread
to prove whether individual small structures are no longer needed.
Plus, in many cases that isn't obvious, and/or it'd be notationally
messy to reclaim things explicitly at a suitable point.

If we were talking about a potentially-very-large data structure,
or one that we might create very many instances of during one
parsing pass, then it might be worth the trouble to free explicitly;
but I don't see that concern applying here.

You might find src/backend/utils/mmgr/README to be worth reading.

            regards, tom lane