To suppress memory leakage in long-lived Lists, lremove() should pfree

the cons cell it's deleting from the list.  Do this, and fix a few callers
that were bogusly assuming it wouldn't free the cons cell.

6 files changed, 51 insertions, 32 deletions

diff --git a/src/backend/nodes/list.c b/src/backend/nodes/list.c
index 6dc6001a0f2..b3c6a18496f 100644
--- a/src/backend/nodes/list.c
+++ b/src/backend/nodes/list.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/nodes/list.c,v 1.42 2002/11/24 21:52:13 tgl Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/nodes/list.c,v 1.43 2002/12/17 01:18:18 tgl Exp $
  *
  * NOTES
  *	  XXX a few of the following functions are duplicated to handle
@@ -269,7 +269,6 @@ llasti(List *l)
  *	Free the List nodes of a list
  *	The pointed-to nodes, if any, are NOT freed.
  *	This works for integer lists too.
- *
  */
 void
 freeList(List *list)
@@ -487,6 +486,7 @@ lremove(void *elem, List *list)
 			result = lnext(l);
 		else
 			lnext(prev) = lnext(l);
+		pfree(l);
 	}
 	return result;
 }
@@ -518,6 +518,7 @@ LispRemove(void *elem, List *list)
 			result = lnext(l);
 		else
 			lnext(prev) = lnext(l);
+		pfree(l);
 	}
 	return result;
 }
@@ -545,6 +546,7 @@ lremovei(int elem, List *list)
 			result = lnext(l);
 		else
 			lnext(prev) = lnext(l);
+		pfree(l);
 	}
 	return result;
 }
diff --git a/src/backend/optimizer/path/pathkeys.c b/src/backend/optimizer/path/pathkeys.c
index af0b61a4034..9c6ed4d1bd3 100644
--- a/src/backend/optimizer/path/pathkeys.c
+++ b/src/backend/optimizer/path/pathkeys.c
@@ -11,7 +11,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/optimizer/path/pathkeys.c,v 1.42 2002/12/12 15:49:32 tgl Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/optimizer/path/pathkeys.c,v 1.43 2002/12/17 01:18:22 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -101,12 +101,17 @@ add_equijoined_keys(Query *root, RestrictInfo *restrictinfo)
 	 */
 	newset = NIL;
 
-	foreach(cursetlink, root->equi_key_list)
+	/* cannot use foreach here because of possible lremove */
+	cursetlink = root->equi_key_list;
+	while (cursetlink)
 	{
 		List	   *curset = lfirst(cursetlink);
 		bool		item1here = member(item1, curset);
 		bool		item2here = member(item2, curset);
 
+		/* must advance cursetlink before lremove possibly pfree's it */
+		cursetlink = lnext(cursetlink);
+
 		if (item1here || item2here)
 		{
 			/*
@@ -128,9 +133,7 @@ add_equijoined_keys(Query *root, RestrictInfo *restrictinfo)
 			newset = set_union(newset, curset);
 
 			/*
-			 * Remove old set from equi_key_list.  NOTE this does not
-			 * change lnext(cursetlink), so the foreach loop doesn't
-			 * break.
+			 * Remove old set from equi_key_list.
 			 */
 			root->equi_key_list = lremove(curset, root->equi_key_list);
 			freeList(curset);	/* might as well recycle old cons cells */
diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c
index aca2c6f4f67..151a37a8889 100644
--- a/src/backend/optimizer/plan/initsplan.c
+++ b/src/backend/optimizer/plan/initsplan.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/optimizer/plan/initsplan.c,v 1.78 2002/12/12 15:49:32 tgl Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/optimizer/plan/initsplan.c,v 1.79 2002/12/17 01:18:25 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -910,13 +910,18 @@ qual_is_redundant(Query *root,
 	do
 	{
 		someadded = false;
-		foreach(olditem, oldquals)
+		/* cannot use foreach here because of possible lremove */
+		olditem = oldquals;
+		while (olditem)
 		{
 			RestrictInfo *oldrinfo = (RestrictInfo *) lfirst(olditem);
 			Node	   *oldleft = (Node *) get_leftop(oldrinfo->clause);
 			Node	   *oldright = (Node *) get_rightop(oldrinfo->clause);
 			Node	   *newguy = NULL;
 
+			/* must advance olditem before lremove possibly pfree's it */
+			olditem = lnext(olditem);
+
 			if (member(oldleft, equalvars))
 				newguy = oldright;
 			else if (member(oldright, equalvars))
@@ -930,8 +935,6 @@ qual_is_redundant(Query *root,
 
 			/*
 			 * Remove this qual from list, since we don't need it anymore.
-			 * Note this doesn't break the foreach() loop, since lremove
-			 * doesn't touch the next-link of the removed cons cell.
 			 */
 			oldquals = lremove(oldrinfo, oldquals);
 		}
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 83a322cf148..e771d666597 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -6,7 +6,7 @@
  * Portions Copyright (c) 1996-2002, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- *	$Header: /cvsroot/pgsql/src/backend/parser/analyze.c,v 1.257 2002/12/13 19:45:56 tgl Exp $
+ *	$Header: /cvsroot/pgsql/src/backend/parser/analyze.c,v 1.258 2002/12/17 01:18:29 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -521,32 +521,38 @@ transformInsertStmt(ParseState *pstate, InsertStmt *stmt,
 	 * Prepare columns for assignment to target table.
 	 */
 	attnos = attrnos;
-	foreach(tl, qry->targetList)
+	/* cannot use foreach here because of possible lremove */
+	tl = qry->targetList;
+	while (tl)
 	{
 		TargetEntry *tle = (TargetEntry *) lfirst(tl);
 		ResTarget  *col;
 
+		/* must advance tl before lremove possibly pfree's it */
+		tl = lnext(tl);
+
 		if (icolumns == NIL || attnos == NIL)
 			elog(ERROR, "INSERT has more expressions than target columns");
+
 		col = (ResTarget *) lfirst(icolumns);
+		Assert(IsA(col, ResTarget));
 
 		/*
 		 * When the value is to be set to the column default we can simply
-		 * drop it now and handle it later on using methods for missing
+		 * drop the TLE now and handle it later on using methods for missing
 		 * columns.
 		 */
-		if (!IsA(tle, InsertDefault))
+		if (IsA(tle, InsertDefault))
 		{
-			Assert(IsA(col, ResTarget));
-			Assert(!tle->resdom->resjunk);
-			updateTargetListEntry(pstate, tle, col->name, lfirsti(attnos),
-								  col->indirection);
+			qry->targetList = lremove(tle, qry->targetList);
+			/* Note: the stmt->cols list is not adjusted to match */
 		}
 		else
 		{
-			icolumns = lremove(icolumns, icolumns);
-			attnos = lremove(attnos, attnos);
-			qry->targetList = lremove(tle, qry->targetList);
+			/* Normal case */
+			Assert(!tle->resdom->resjunk);
+			updateTargetListEntry(pstate, tle, col->name, lfirsti(attnos),
+								  col->indirection);
 		}
 
 		icolumns = lnext(icolumns);
@@ -554,8 +560,9 @@ transformInsertStmt(ParseState *pstate, InsertStmt *stmt,
 	}
 
 	/*
-	 * Ensure that the targetlist has the same number of  entries that
-	 * were present in the columns list.  Don't do the check for select
+	 * Ensure that the targetlist has the same number of entries that
+	 * were present in the columns list.  Don't do the check unless
+	 * an explicit columns list was given, though.
 	 * statements.
 	 */
 	if (stmt->cols != NIL && (icolumns != NIL || attnos != NIL))
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index d0badcc154f..c19d15b4f02 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/rewrite/rewriteHandler.c,v 1.114 2002/12/12 15:49:40 tgl Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/rewrite/rewriteHandler.c,v 1.115 2002/12/17 01:18:32 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -205,9 +205,11 @@ adjustJoinTreeList(Query *parsetree, bool removert, int rt_index)
 		{
 			RangeTblRef *rtr = lfirst(jjt);
 
-			if (IsA(rtr, RangeTblRef) &&rtr->rtindex == rt_index)
+			if (IsA(rtr, RangeTblRef) &&
+				rtr->rtindex == rt_index)
 			{
 				newjointree = lremove(rtr, newjointree);
+				/* foreach is safe because we exit loop after lremove... */
 				break;
 			}
 		}
diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index c378f8b50e3..58c63c210cc 100644
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -15,7 +15,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/utils/adt/selfuncs.c,v 1.123 2002/12/12 15:49:40 tgl Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/utils/adt/selfuncs.c,v 1.124 2002/12/17 01:18:35 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1953,10 +1953,15 @@ estimate_num_groups(Query *root, List *groupClauses, double input_rows)
 		if (HeapTupleIsValid(statsTuple))
 			ReleaseSysCache(statsTuple);
 
-		foreach(l2, varinfos)
+		/* cannot use foreach here because of possible lremove */
+		l2 = varinfos;
+		while (l2)
 		{
 			MyVarInfo  *varinfo = (MyVarInfo *) lfirst(l2);
 
+			/* must advance l2 before lremove possibly pfree's it */
+			l2 = lnext(l2);
+
 			if (var->varno != varinfo->var->varno &&
 				vars_known_equal(root, var, varinfo->var))
 			{
@@ -1969,10 +1974,7 @@ estimate_num_groups(Query *root, List *groupClauses, double input_rows)
 				}
 				else
 				{
-					/*
-					 * Delete the older item.  We assume lremove() will not
-					 * break the lnext link of the item...
-					 */
+					/* Delete the older item */
 					varinfos = lremove(varinfo, varinfos);
 				}
 			}