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 [email protected] 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);
    }