ALTER TABLE ADD COLUMN exhibits a significant memory leak when adding a

column with a default expression. In that situation, we need to rewrite
the heap relation. To evaluate the new default expression, we use
ExecEvalExpr(); however, this can allocate memory in the current memory
context, and ATRewriteTable() does not switch out of the active portal's
heap memory context. The end result is a rather large memory leak (on
the order of gigabytes for a reasonably sized table).

This patch changes ATRewriteTable() to switch to the per-tuple memory
context before beginning the per-tuple loop. It also removes an explicit
heap_freetuple() in the loop, since that is no longer needed.

In an unrelated change, I noticed the code was scanning through the
attributes of the new tuple descriptor for each tuple of the old table.
I changed this to use precomputation, which should slightly speed up
the loop.

Thanks to steve@deefs.net for reporting the leak.

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 4f96902beabee6ba3cc1639ee9f9f0eceb479e97..fddfd13c71671e1318a5cf69016e0558d5b8a5df 100644 (file)
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/commands/tablecmds.c,v 1.145 2005/01/27 23:23:55 neilc Exp $
+ *       $PostgreSQL: pgsql/src/backend/commands/tablecmds.c,v 1.146 2005/02/09 23:17:26 neilc Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -2460,6 +2460,9 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap)
                TupleTableSlot *newslot;
                HeapScanDesc scan;
                HeapTuple       tuple;
+               MemoryContext oldCxt;
+               List *dropped_attrs = NIL;
+               ListCell *lc;
 
                econtext = GetPerTupleExprContext(estate);
 
@@ -2480,29 +2483,40 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap)
                memset(values, 0, i * sizeof(Datum));
                memset(nulls, 'n', i * sizeof(char));
 
+               /*
+                * Any attributes that are dropped according to the new tuple
+                * descriptor can be set to NULL. We precompute the list of
+                * dropped attributes to avoid needing to do so in the
+                * per-tuple loop.
+                */
+               for (i = 0; i < newTupDesc->natts; i++)
+               {
+                       if (newTupDesc->attrs[i]->attisdropped)
+                               dropped_attrs = lappend_int(dropped_attrs, i);
+               }
+
                /*
                 * Scan through the rows, generating a new row if needed and then
                 * checking all the constraints.
                 */
                scan = heap_beginscan(oldrel, SnapshotNow, 0, NULL);
 
+               /*
+                * Switch to per-tuple memory context and reset it for each
+                * tuple produced, so we don't leak memory.
+                */
+               oldCxt = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
+
                while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
                {
                        if (newrel)
                        {
-                               /*
-                                * Extract data from old tuple.  We can force to null any
-                                * columns that are deleted according to the new tuple.
-                                */
-                               int                     natts = newTupDesc->natts;
-
+                               /* Extract data from old tuple */
                                heap_deformtuple(tuple, oldTupDesc, values, nulls);
 
-                               for (i = 0; i < natts; i++)
-                               {
-                                       if (newTupDesc->attrs[i]->attisdropped)
-                                               nulls[i] = 'n';
-                               }
+                               /* Set dropped attributes to null in new tuple */
+                               foreach (lc, dropped_attrs)
+                                       nulls[lfirst_int(lc)] = 'n';
 
                                /*
                                 * Process supplied expressions to replace selected
@@ -2526,6 +2540,11 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap)
                                                nulls[ex->attnum - 1] = ' ';
                                }
 
+                               /*
+                                * Form the new tuple. Note that we don't explicitly
+                                * pfree it, since the per-tuple memory context will
+                                * be reset shortly.
+                                */
                                tuple = heap_formtuple(newTupDesc, values, nulls);
                        }
 
@@ -2572,17 +2591,14 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap)
 
                        /* Write the tuple out to the new relation */
                        if (newrel)
-                       {
                                simple_heap_insert(newrel, tuple);
 
-                               heap_freetuple(tuple);
-                       }
-
                        ResetExprContext(econtext);
 
                        CHECK_FOR_INTERRUPTS();
                }
 
+               MemoryContextSwitchTo(oldCxt);
                heap_endscan(scan);
        }