Skip site navigation (1) Skip section navigation (2)

Re: COPY and default values

From: Neil Conway <nconway(at)klamath(dot)dyndns(dot)org>
To: "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: pgsql-patches(at)postgresql(dot)org
Subject: Re: COPY and default values
Date: 2002-05-27 23:10:53
Message-ID: 20020527191053.592cb219.nconway@klamath.dyndns.org (view raw or flat)
Thread:
Lists: pgsql-patches
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

Attachment: copy-def-value-3.patch
Description: application/octet-stream (22.4 KB)

In response to

Responses

pgsql-patches by date

Next:From: Tom LaneDate: 2002-05-27 23:28:30
Subject: Re: COPY and default values
Previous:From: Bear GilesDate: 2002-05-27 22:14:44
Subject: Re: SSL (patch 5)

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group