Re: COPY and default values

On Mon, 27 May 2002 17:15:21 -0400
"Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Neil Conway <nconway(at)klamath(dot)dyndns(dot)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...

> [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.)

Woops :-) Ok, removed that. Actually, I had just inserted that when
I was trying to debug the assertion failure I mentioned earlier.

> 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.

Can you elaborate on exactly how you'd like to see COPY's behavior
changed? What's the rationale for disallowing missing fields in
COPY input?

> 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.

Good spot -- there's a special case for that now.

> 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...

OK, removed them.

> 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.

Oh, yeah -- I forgot to mention that. I wasn't really sure where that
code should go, so I basically picked execUtils.c at random -- if you'd
prefer that I move the code to somewhere in catalog/ that's fine,
just let me know where. I don't have a strong preference myself.

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

Thanks Tom. However, after applying the changes noted above, the test
case still fails (and I'm still scratching my head about what's
causing the problem). A new version of the patch is attached.

Cheers,

Neil

--
Neil Conway <neilconway(at)rogers(dot)com>
PGP Key ID: DB3C29FC