Re: COPY and default values - Mailing list pgsql-patches

Neil Conway <nconway@klamath.dyndns.org> writes:
> ERROR:  copy: line 2, Memory exhausted in AllocSetAlloc(1065320379)

> I think I've mismanaged the memory contexts involved somehow, but
> I'm not sure what the problem is. Any help would be appreciated...

Well, for one thing you're doing

+         for (i = 0; i < attr_count; i++)
+         {
+             if (nulls[i] == 'd' && values[i] == 0)
+             {
+                 bool isNull;
+
+                 values[i] = ExecEvalExprSwitchContext(defaults[i], econtext,
+                                                       &isNull, NULL);
+
+                 /* If it's NULL, return value is meaningless */
+                 if (isNull)
+                 {
+                     values[i] = 0;
+                     nulls[i] = 'n';
+                 }
+                 else
+                     nulls[i] = ' ';
+
+                 ResetPerTupleExprContext(estate);
+             }
+         }

which means you reset the memory context containing the default results
before those results can ever get used, leaving dangling pointers in
values[i].  (Hint: there's a reason why the econtext is called the
"per-tuple" context, not "per-column" context.  The one reset that's in
there now is sufficient.)

The nulls = 'd' mechanism is ugly and unnecessary, I think.  We were
intending to modify COPY's behavior to prohibit missing/extra fields
in incoming rows anyway, so there's no reason not to know in advance
exactly which columns need to have defaults inserted, and to set up
default info for only those columns.  I'm also somewhat uncomfortable
with the fact that the patch invokes ExecEvalExpr on a NULL pointer
if there is a default-less column involved --- perhaps that works at
the moment but I don't like it.  Should special-case that.

The pfree's you've added near line 1021 look rather pointless, seeing
as how (a) they're only pfree'ing the topmost node of the default
expressions, and (b) the whole context is about to get reset anyhow.
There isn't a lot of percentage in any of those end-of-statement
pfrees that are there now...

I didn't like the aspect of the patch that moves build_column_default
into execUtils.c.  It's not an executor routine (as evidenced by the
fact that you had to add a pile of new inclusions to execUtils.c to
make it compile).  I'm not sure of the cleanest place for it; maybe
someplace in backend/catalog/, though really there's nothing wrong with
keeping it in the rewriter.

I haven't tried to run the patch, but those were some things I noticed
in a quick eyeball review...

            regards, tom lane