list_free in addRangeTableEntryForJoin

Started by Ilia Evdokimovover 1 year ago7 messages

ilya.evdokimov@tantorlabs.com

over 1 year ago

1 attachment(s)

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

Attachments:

list_free.patchtext/x-patch; charset=UTF-8; name=list_free.patchDownload

From cd7aa7ac5904501085a980944dc3c18f42721c06 Mon Sep 17 00:00:00 2001
From: Ilia Evdokimov <ilya.evdokimov@tantorlabs.com>
Date: Mon, 10 Jun 2024 13:10:31 +0300
Subject: [PATCH] After concatenation two lists where the second one is from
 list_copy_tail do not free it

---
 src/backend/parser/parse_relation.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c
index 2f64eaf0e3..8230cf27cc 100644
--- a/src/backend/parser/parse_relation.c
+++ b/src/backend/parser/parse_relation.c
@@ -2258,8 +2258,11 @@ addRangeTableEntryForJoin(ParseState *pstate,
 
 	/* fill in any unspecified alias columns */
 	if (numaliases < list_length(colnames))
+	{
 		eref->colnames = list_concat(eref->colnames,
 									 list_copy_tail(colnames, numaliases));
+		list_free(colnames);
+	}
 
 	if (numaliases > list_length(colnames))
 		ereport(ERROR,
-- 
2.34.1

Ranier Vilela

ranier.vf@gmail.com

over 1 year ago

In reply to: Ilia Evdokimov (#1)

Re: list_free in addRangeTableEntryForJoin

Em seg., 10 de jun. de 2024 às 07:35, Ilia Evdokimov <
ilya.evdokimov@tantorlabs.com> escreveu:

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.

list_copy_tail actually makes a new copy.
But callers of addRangeTableEntryForJoin,
expects to handle a list or NIL, if we free the memory,
Shouldn't I set the variable *colnames* to NIL, too?

best regards,
Ranier Vilela

Ilia Evdokimov

ilya.evdokimov@tantorlabs.com

over 1 year ago

In reply to: Ilia Evdokimov (#1)

1 attachment(s)

Re: list_free in addRangeTableEntryForJoin

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

Attachments:

0002-list-free.patchtext/x-patch; charset=UTF-8; name=0002-list-free.patchDownload

From 853c5bc854bcc03e791cf32cc8cad7b257ec558f Mon Sep 17 00:00:00 2001
From: Ilia Evdokimov <ilya.evdokimov@tantorlabs.ru>
Date: Mon, 10 Jun 2024 15:09:12 +0300
Subject: [PATCH] After concatenation two lists where the second one is from
 list_copy_tail do not free it

---
 src/backend/parser/analyze.c      | 2 ++
 src/backend/parser/parse_clause.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 28fed9d87f..c0708a6e3e 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -1943,6 +1943,8 @@ transformSetOperationStmt(ParseState *pstate, SelectStmt *stmt)
 	if (pstate->p_hasAggs || qry->groupClause || qry->groupingSets || qry->havingQual)
 		parseCheckAggregates(pstate, qry);
 
+	list_free(targetnames);
+
 	return qry;
 }
 
diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index 8118036495..25f8c84a9f 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -1620,6 +1620,8 @@ transformFromClauseItem(ParseState *pstate, Node *n,
 		*top_nsitem = nsitem;
 		*namespace = lappend(my_namespace, nsitem);
 
+		list_free(res_colnames);
+
 		return (Node *) j;
 	}
 	else
-- 
2.34.1

Ranier Vilela

ranier.vf@gmail.com

over 1 year ago

In reply to: Ilia Evdokimov (#3)

Re: list_free in addRangeTableEntryForJoin

Em seg., 10 de jun. de 2024 às 09:11, Ilia Evdokimov <
ilya.evdokimov@tantorlabs.com> escreveu:

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.

Seems like a better style.

Now you need to analyze whether the memory in question is not managed by a
Context and released in a block,
which would make this release useless.

best regards,
Ranier Vilela

Ilia Evdokimov

ilya.evdokimov@tantorlabs.com

over 1 year ago

In reply to: Ilia Evdokimov (#3)

1 attachment(s)

Re: list_free in addRangeTableEntryForJoin

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

Attachments:

0003-list-free.patchtext/x-patch; charset=UTF-8; name=0003-list-free.patchDownload

From b545770d57ac3ca137e9ad97c004576e77213648 Mon Sep 17 00:00:00 2001
From: Ilia Evdokimov <ilya.evdokimov@tantorlabs.ru>
Date: Mon, 10 Jun 2024 16:39:14 +0300
Subject: [PATCH] After concatenation two lists where the second one is from
 list_copy_tail do not free it

---
 src/backend/parser/analyze.c      | 2 ++
 src/backend/parser/parse_clause.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 28fed9d87f..50c47a64ed 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -1880,6 +1880,8 @@ transformSetOperationStmt(ParseState *pstate, SelectStmt *stmt)
 										NULL,
 										false);
 
+   list_free(targetnames);
+
 	sv_namespace = pstate->p_namespace;
 	pstate->p_namespace = NIL;
 
diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index 8118036495..2fa6c03be7 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -1552,6 +1552,8 @@ transformFromClauseItem(ParseState *pstate, Node *n,
 										   j->alias,
 										   true);
 
+       list_free(res_colnames);
+
 		/* Verify that we correctly predicted the join's RT index */
 		Assert(j->rtindex == nsitem->p_rtindex);
 		/* Cross-check number of columns, too */
-- 
2.34.1

Ranier Vilela

ranier.vf@gmail.com

over 1 year ago

In reply to: Ilia Evdokimov (#5)

Re: list_free in addRangeTableEntryForJoin

Em seg., 10 de jun. de 2024 às 10:45, Ilia Evdokimov <
ilya.evdokimov@tantorlabs.com> escreveu:

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,

You can see this line?
sortnscolumns = (ParseNamespaceColumn *)
palloc0

All allocations in the backend occur at Context memory managers.
Resource leak can occur mainly with TopTransactionContext.

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.

This style is not recommended.
You prevent the use of colnames after calling addRangeTableEntryForJoin.

Better free at the end of the function, like 0002.

Tip 0001, 0002, 0003 numerations are to different patchs.
v1, v2, v3 are new versions of the same patch.

best regards,
Ranier Vilela

Tom Lane

tgl@sss.pgh.pa.us

over 1 year ago

In reply to: Ilia Evdokimov (#1)

Re: list_free in addRangeTableEntryForJoin

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