Skip to content

P
Postgres FD Implementation
Commit 745f0c21 authored by Bruce Momjian's avatar Bruce Momjian
Browse files
Options

Remove compiler warning about uninitialized warnings.

parent bf74477b
Hide whitespace changes
Inline Side-by-side
Showing with 2 additions and 1593 deletions
doc/TODO.detail/flock deleted 100644 → 0
View file @ bf74477b
From tgl@sss.pgh.pa.us Sun Aug 30 11:25:23 1998
Received: from sss.sss.pgh.pa.us (sss.pgh.pa.us [206.210.65.6])
by candle.pha.pa.us (8.8.5/8.8.5) with ESMTP id LAA12607
for <maillist@candle.pha.pa.us>; Sun, 30 Aug 1998 11:25:20 -0400 (EDT)
Received: from sss.sss.pgh.pa.us (localhost [127.0.0.1])
by sss.sss.pgh.pa.us (8.9.1/8.9.1) with ESMTP id LAA15788;
Sun, 30 Aug 1998 11:23:38 -0400 (EDT)
To: Bruce Momjian <maillist@candle.pha.pa.us>
cc: dz@cs.unitn.it (Massimo Dal Zotto), hackers@postgreSQL.org
Subject: Re: [HACKERS] flock patch breaks things here
In-reply-to: Your message of Sun, 30 Aug 1998 08:19:52 -0400 (EDT)
<199808301219.IAA08821@candle.pha.pa.us>
Date: Sun, 30 Aug 1998 11:23:38 -0400
Message-ID: <15786.904490618@sss.pgh.pa.us>
From: Tom Lane <tgl@sss.pgh.pa.us>
Status: RO
Bruce Momjian <maillist@candle.pha.pa.us> writes:
> Can't we just have configure check for flock(). Another idea is to
> create a 'pid' file in the pgsql/data/base directory, and do a kill -0
> to see if it is stil running before removing the lock.
The latter approach is what I was going to suggest. Writing a pid file
would be a fine idea anyway --- for one thing, it makes it a lot easier
to write a "kill the postmaster" script. Given that the postmaster
should write a pid file, a new postmaster should look for an existing
pid file, and try to do a kill(pid, 0) on the number contained therein.
If this doesn't return an error, then you figure there is already a
postmaster running, complain, and exit. Otherwise you figure you is it,
(re)write the pid file and away you go. Then pqcomm.c can just
unconditionally delete any old file that's in the way of making the
pipe.
The pidfile checking and creation probably ought to go in postmaster.c,
not down inside pqcomm.c. I never liked the fact that a critical
interlock function was being done by a low-level library that one might
not even want to invoke (if all your clients are using TCP, opening up
the Unix-domain socket is a waste of time, no?).
BTW, there is another problem with relying on flock on the socket file
for this purpose: it opens up a hole for a denial-of-service attack.
Anyone who can write the file can flock it. (We already had a problem
with DOS via creating a dummy file at /tmp/.s.PGSQL.5432, but it would
be harder to spot the culprit with an flock-based interference.)
regards, tom lane
From owner-pgsql-hackers@hub.org Sun Aug 30 12:27:41 1998
Received: from hub.org (hub.org [209.47.148.200])
by candle.pha.pa.us (8.8.5/8.8.5) with ESMTP id MAA12976
for <maillist@candle.pha.pa.us>; Sun, 30 Aug 1998 12:27:37 -0400 (EDT)
Received: from localhost (majordom@localhost) by hub.org (8.8.8/8.7.5) with SMTP id MAA09234; Sun, 30 Aug 1998 12:24:51 -0400 (EDT)
Received: by hub.org (TLB v0.10a (1.23 tibbs 1997/01/09 00:29:32)); Sun, 30 Aug 1998 12:23:26 +0000 (EDT)
Received: (from majordom@localhost) by hub.org (8.8.8/8.7.5) id MAA09167 for pgsql-hackers-outgoing; Sun, 30 Aug 1998 12:23:25 -0400 (EDT)
Received: from mambo.cs.unitn.it (mambo.cs.unitn.it [193.205.199.204]) by hub.org (8.8.8/8.7.5) with SMTP id MAA09150 for <hackers@postgreSQL.org>; Sun, 30 Aug 1998 12:23:08 -0400 (EDT)
Received: from boogie.cs.unitn.it (dz@boogie [193.205.199.79]) by mambo.cs.unitn.it (8.6.12/8.6.12) with ESMTP id SAA29572; Sun, 30 Aug 1998 18:21:42 +0200
Received: (from dz@localhost) by boogie.cs.unitn.it (8.8.5/8.6.9) id SAA05993; Sun, 30 Aug 1998 18:21:41 +0200
From: Massimo Dal Zotto <dz@cs.unitn.it>
Message-Id: <199808301621.SAA05993@boogie.cs.unitn.it>
Subject: Re: [HACKERS] flock patch breaks things here
To: hackers@postgreSQL.org (PostgreSQL Hackers)
Date: Sun, 30 Aug 1998 18:21:41 +0200 (MET DST)
Cc: tgl@sss.pgh.pa.us (Tom Lane)
In-Reply-To: <15786.904490618@sss.pgh.pa.us> from "Tom Lane" at Aug 30, 98 11:23:38 am
X-Mailer: ELM [version 2.4 PL24 ME4]
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Transfer-Encoding: 8bit
Sender: owner-pgsql-hackers@hub.org
Precedence: bulk
Status: ROr
>
> Bruce Momjian <maillist@candle.pha.pa.us> writes:
> > Can't we just have configure check for flock(). Another idea is to
> > create a 'pid' file in the pgsql/data/base directory, and do a kill -0
> > to see if it is stil running before removing the lock.
>
> The latter approach is what I was going to suggest. Writing a pid file
> would be a fine idea anyway --- for one thing, it makes it a lot easier
> to write a "kill the postmaster" script. Given that the postmaster
> should write a pid file, a new postmaster should look for an existing
> pid file, and try to do a kill(pid, 0) on the number contained therein.
> If this doesn't return an error, then you figure there is already a
> postmaster running, complain, and exit. Otherwise you figure you is it,
> (re)write the pid file and away you go. Then pqcomm.c can just
> unconditionally delete any old file that's in the way of making the
> pipe.
>
> The pidfile checking and creation probably ought to go in postmaster.c,
> not down inside pqcomm.c. I never liked the fact that a critical
> interlock function was being done by a low-level library that one might
> not even want to invoke (if all your clients are using TCP, opening up
> the Unix-domain socket is a waste of time, no?).
>
> BTW, there is another problem with relying on flock on the socket file
> for this purpose: it opens up a hole for a denial-of-service attack.
> Anyone who can write the file can flock it. (We already had a problem
> with DOS via creating a dummy file at /tmp/.s.PGSQL.5432, but it would
> be harder to spot the culprit with an flock-based interference.)
This came to my mind, but I didn't think this would have happened so
quickly. In my opinion the socket and the pidfile should be created in a
directory owned by postgres, for example /tmp/.Pgsql-unix, like does X.
--
Massimo Dal Zotto
+----------------------------------------------------------------------+
| Massimo Dal Zotto email: dz@cs.unitn.it |
| Via Marconi, 141 phone: ++39-461-534251 |
| 38057 Pergine Valsugana (TN) www: http://www.cs.unitn.it/~dz/ |
| Italy pgp: finger dz@tango.cs.unitn.it |
+----------------------------------------------------------------------+
From owner-pgsql-hackers@hub.org Sun Aug 30 13:01:10 1998
Received: from renoir.op.net (root@renoir.op.net [209.152.193.4])
by candle.pha.pa.us (8.8.5/8.8.5) with ESMTP id NAA13785
for <maillist@candle.pha.pa.us>; Sun, 30 Aug 1998 13:01:09 -0400 (EDT)
Received: from hub.org (hub.org [209.47.148.200]) by renoir.op.net (o1/$ Revision: 1.18 $) with ESMTP id MAA29386 for <maillist@candle.pha.pa.us>; Sun, 30 Aug 1998 12:58:24 -0400 (EDT)
Received: from localhost (majordom@localhost) by hub.org (8.8.8/8.7.5) with SMTP id MAA11406; Sun, 30 Aug 1998 12:54:48 -0400 (EDT)
Received: by hub.org (TLB v0.10a (1.23 tibbs 1997/01/09 00:29:32)); Sun, 30 Aug 1998 12:52:22 +0000 (EDT)
Received: (from majordom@localhost) by hub.org (8.8.8/8.7.5) id MAA11310 for pgsql-hackers-outgoing; Sun, 30 Aug 1998 12:52:20 -0400 (EDT)
Received: from sss.sss.pgh.pa.us (sss.pgh.pa.us [206.210.65.6]) by hub.org (8.8.8/8.7.5) with ESMTP id MAA11296 for <hackers@postgreSQL.org>; Sun, 30 Aug 1998 12:52:13 -0400 (EDT)
Received: from sss.sss.pgh.pa.us (localhost [127.0.0.1])
by sss.sss.pgh.pa.us (8.9.1/8.9.1) with ESMTP id MAA16094;
Sun, 30 Aug 1998 12:50:55 -0400 (EDT)
To: Massimo Dal Zotto <dz@cs.unitn.it>
cc: hackers@postgreSQL.org (PostgreSQL Hackers)
Subject: Re: [HACKERS] flock patch breaks things here
In-reply-to: Your message of Sun, 30 Aug 1998 18:21:41 +0200 (MET DST)
<199808301621.SAA05993@boogie.cs.unitn.it>
Date: Sun, 30 Aug 1998 12:50:55 -0400
Message-ID: <16092.904495855@sss.pgh.pa.us>
From: Tom Lane <tgl@sss.pgh.pa.us>
Sender: owner-pgsql-hackers@hub.org
Precedence: bulk
Status: RO
Massimo Dal Zotto <dz@cs.unitn.it> writes:
> In my opinion the socket and the pidfile should be created in a
> directory owned by postgres, for example /tmp/.Pgsql-unix, like does X.
The pidfile belongs at the top level of the database directory (eg,
/usr/local/pgsql/data/postmaster.pid), because what it actually
represents is that there is a postmaster running *for that database
group*.
If you want to support multiple database sets on one machine (which I
do), then the interlock has to be per database directory. Putting the
pidfile into a common directory would mean we'd have to invent some
kind of pidfile naming convention to keep multiple postmasters from
tromping on each other. This is unnecessarily complex.
I agree with you that putting the socket file into a less easily munged
directory than /tmp would be a good idea for security. But that's a
separate issue. On machines that understand stickybits for directories,
the security hole is not really very big.
At this point, the fact that /tmp/.s.PGSQL.port# is the socket path is
effectively a version-independent aspect of the FE/BE protocol, and so
we can't change it without breaking old applications. I'm not sure that
that's worth the security improvement.
What I'd like to see someday is a postmaster command line switch to tell
it to use *only* TCP connections and not create a Unix socket at all.
That hasn't been possible so far, because we were relying on the socket
file to provide a safety interlock against starting multiple
postmasters. But an interlock using a pidfile would be much better.
(Look around; *every* other Unix daemon I know of that wants to ensure
that there's only one of it uses a pidfile interlock. Not file locking.
There's a reason why that's the well-trodden path.)
regards, tom lane
From owner-pgsql-hackers@hub.org Sun Aug 30 15:31:13 1998
Received: from hub.org (hub.org [209.47.148.200])
by candle.pha.pa.us (8.8.5/8.8.5) with ESMTP id PAA15275
for <maillist@candle.pha.pa.us>; Sun, 30 Aug 1998 15:31:11 -0400 (EDT)
Received: from localhost (majordom@localhost) by hub.org (8.8.8/8.7.5) with SMTP id PAA22194; Sun, 30 Aug 1998 15:27:20 -0400 (EDT)
Received: by hub.org (TLB v0.10a (1.23 tibbs 1997/01/09 00:29:32)); Sun, 30 Aug 1998 15:23:58 +0000 (EDT)
Received: (from majordom@localhost) by hub.org (8.8.8/8.7.5) id PAA21800 for pgsql-hackers-outgoing; Sun, 30 Aug 1998 15:23:57 -0400 (EDT)
Received: from thelab.hub.org (nat0118.mpoweredpc.net [142.177.188.118]) by hub.org (8.8.8/8.7.5) with ESMTP id PAA21696 for <hackers@postgreSQL.org>; Sun, 30 Aug 1998 15:22:51 -0400 (EDT)
Received: from localhost (scrappy@localhost)
by thelab.hub.org (8.9.1/8.8.8) with SMTP id QAA18542;
Sun, 30 Aug 1998 16:21:29 -0300 (ADT)
(envelope-from scrappy@hub.org)
X-Authentication-Warning: thelab.hub.org: scrappy owned process doing -bs
Date: Sun, 30 Aug 1998 16:21:28 -0300 (ADT)
From: The Hermit Hacker <scrappy@hub.org>
To: Tom Lane <tgl@sss.pgh.pa.us>
cc: Massimo Dal Zotto <dz@cs.unitn.it>,
PostgreSQL Hackers <hackers@postgreSQL.org>
Subject: Re: [HACKERS] flock patch breaks things here
In-Reply-To: <16092.904495855@sss.pgh.pa.us>
Message-ID: <Pine.BSF.4.02.9808301618350.343-100000@thelab.hub.org>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Sender: owner-pgsql-hackers@hub.org
Precedence: bulk
Status: RO
On Sun, 30 Aug 1998, Tom Lane wrote:
> Massimo Dal Zotto <dz@cs.unitn.it> writes:
> > In my opinion the socket and the pidfile should be created in a
> > directory owned by postgres, for example /tmp/.Pgsql-unix, like does X.
>
> The pidfile belongs at the top level of the database directory (eg,
> /usr/local/pgsql/data/postmaster.pid), because what it actually
> represents is that there is a postmaster running *for that database
> group*.
I have to agree with this one...but then it also negates the
argument about the flock() DoS...*grin*
BTW...I like the kill(pid,0) solution myself, primarily because it
is, i think, the most portable solution.
I would not consider a patch to remove the flock() solution and
replace it with the kill(pid,0) solution a new feature, just an
improvement of an existing one...either way, moving the pid file (or
socket, for that matter) from /tmp should be listed as a security related
requirement for v6.4 :)
Marc G. Fournier
Systems Administrator @ hub.org
primary: scrappy@hub.org secondary: scrappy@{freebsd|postgresql}.org
From owner-pgsql-hackers@hub.org Sun Aug 30 22:41:10 1998
Received: from hub.org (hub.org [209.47.148.200])
by candle.pha.pa.us (8.8.5/8.8.5) with ESMTP id WAA01526
for <maillist@candle.pha.pa.us>; Sun, 30 Aug 1998 22:41:08 -0400 (EDT)
Received: from localhost (majordom@localhost) by hub.org (8.8.8/8.7.5) with SMTP id WAA29298; Sun, 30 Aug 1998 22:38:18 -0400 (EDT)
Received: by hub.org (TLB v0.10a (1.23 tibbs 1997/01/09 00:29:32)); Sun, 30 Aug 1998 22:35:05 +0000 (EDT)
Received: (from majordom@localhost) by hub.org (8.8.8/8.7.5) id WAA29203 for pgsql-hackers-outgoing; Sun, 30 Aug 1998 22:35:03 -0400 (EDT)
Received: from sss.sss.pgh.pa.us (sss.pgh.pa.us [206.210.65.6]) by hub.org (8.8.8/8.7.5) with ESMTP id WAA29017 for <hackers@postgreSQL.org>; Sun, 30 Aug 1998 22:34:55 -0400 (EDT)
Received: from sss.sss.pgh.pa.us (localhost [127.0.0.1])
by sss.sss.pgh.pa.us (8.9.1/8.9.1) with ESMTP id WAA20075;
Sun, 30 Aug 1998 22:34:41 -0400 (EDT)
To: The Hermit Hacker <scrappy@hub.org>
cc: PostgreSQL Hackers <hackers@postgreSQL.org>
Subject: Re: [HACKERS] flock patch breaks things here
In-reply-to: Your message of Sun, 30 Aug 1998 16:21:28 -0300 (ADT)
<Pine.BSF.4.02.9808301618350.343-100000@thelab.hub.org>
Date: Sun, 30 Aug 1998 22:34:40 -0400
Message-ID: <20073.904530880@sss.pgh.pa.us>
From: Tom Lane <tgl@sss.pgh.pa.us>
Sender: owner-pgsql-hackers@hub.org
Precedence: bulk
Status: ROr
The Hermit Hacker <scrappy@hub.org> writes:
> either way, moving the pid file (or
> socket, for that matter) from /tmp should be listed as a security related
> requirement for v6.4 :)
Huh? There is no pid file being generated in /tmp (or anywhere else)
at the moment. If we do add one, it should not go into /tmp for the
reasons I gave before.
Where the Unix-domain socket file lives is an entirely separate issue.
If we move the socket out of /tmp then we have just kicked away all the
work we did to preserve backwards compatibility of the FE/BE protocol
with existing clients. Being able to talk to a 1.0 client isn't much
good if you aren't listening where he's going to try to contact you.
So I think I have to vote in favor of leaving the socket where it is.
regards, tom lane
From owner-pgsql-hackers@hub.org Mon Aug 31 11:31:19 1998
Received: from renoir.op.net (root@renoir.op.net [209.152.193.4])
by candle.pha.pa.us (8.8.5/8.8.5) with ESMTP id LAA21195
for <maillist@candle.pha.pa.us>; Mon, 31 Aug 1998 11:31:13 -0400 (EDT)
Received: from hub.org (hub.org [209.47.148.200]) by renoir.op.net (o1/$ Revision: 1.18 $) with ESMTP id LAA06827 for <maillist@candle.pha.pa.us>; Mon, 31 Aug 1998 11:17:41 -0400 (EDT)
Received: from localhost (majordom@localhost) by hub.org (8.8.8/8.7.5) with SMTP id LAA24792; Mon, 31 Aug 1998 11:12:18 -0400 (EDT)
Received: by hub.org (TLB v0.10a (1.23 tibbs 1997/01/09 00:29:32)); Mon, 31 Aug 1998 11:10:31 +0000 (EDT)
Received: (from majordom@localhost) by hub.org (8.8.8/8.7.5) id LAA24742 for pgsql-hackers-outgoing; Mon, 31 Aug 1998 11:10:29 -0400 (EDT)
Received: from trillium.nmsu.edu (trillium.NMSU.Edu [128.123.5.15]) by hub.org (8.8.8/8.7.5) with ESMTP id LAA24725 for <hackers@postgreSQL.org>; Mon, 31 Aug 1998 11:10:22 -0400 (EDT)
Received: (from brook@localhost)
by trillium.nmsu.edu (8.8.8/8.8.8) id JAA03282;
Mon, 31 Aug 1998 09:09:01 -0600 (MDT)
Date: Mon, 31 Aug 1998 09:09:01 -0600 (MDT)
Message-Id: <199808311509.JAA03282@trillium.nmsu.edu>
From: Brook Milligan <brook@trillium.NMSU.Edu>
To: tgl@sss.pgh.pa.us
CC: dg@informix.com, hackers@postgreSQL.org
In-reply-to: <23042.904573041@sss.pgh.pa.us> (message from Tom Lane on Mon, 31
Aug 1998 10:17:21 -0400)
Subject: Re: [HACKERS] flock patch breaks things here
References: <23042.904573041@sss.pgh.pa.us>
Sender: owner-pgsql-hackers@hub.org
Precedence: bulk
Status: ROr
I just came up with an idea that might help alleviate the /tmp security
exposure without creating a backwards-compatibility problem. It works
like this:
1. During installation, create a subdirectory of /tmp to hold Postgres'
socket files and associated pid lockfiles. This subdirectory should be
owned by the Postgres superuser and have permissions 755
(world-readable, writable only by Postgres superuser). Maybe call it
/tmp/.pgsql --- the name should start with a dot to keep it out of the
way. (Bruce points out that some systems clear /tmp during reboot, so
it might be that a postmaster will have to be prepared to recreate this
directory at startup --- anyone know if subdirectories of /tmp are
zapped too? My system doesn't do that...)
...
I notice that on my system, the X11 socket files in /tmp/.X11-unix are
actually symlinks to socket files in /usr/spool/sockets/X11. I dunno if
it's worth our trouble to get into putting our sockets under /usr/spool
or /var/spool or whatever --- seems like another configuration choice to
mess up. It'd be nice if the socket directory lived somewhere where the
parent dirs weren't world-writable, but this would mean one more thing
that you have to have root permissions for in order to install pgsql.
It seems like we need a directory for locks (= pid files) and one for
sockets (perhaps the same one). I strongly suggest that the location
for these be configurable. By default, it might make sense to put
them in ~pgsql/locks and ~pgsql/sockets. It is easy (i.e., I'll be
glad to do it) to modify configure.in to take options like
--lock-dir=/var/spool/lock
--socket-dir=/var/spool/sockets
that set cc defines and have the code respond accordingly. This way,
those who don't care (or don't have root access) can use the defaults,
whereas those with root access who like to keep locks and sockets in a
common place can do so easily. Either way, multiple postmasters (all
compiled with the same options of course) can check the appropriate
locks in the well-known places. Finally, drop the link into /tmp for
the old socket and document that it will be disappearing at some
point, and all is fine.
If someone wants to give me some guidance on what preprocessor
variables should be set in response to the above options (or something
like them), I'll do the configure stuff.
Cheers,
Brook
doc/TODO.detail/memory deleted 100644 → 0
View file @ bf74477b
From owner-pgsql-hackers@hub.org Thu Nov 26 08:31:13 1998
Received: from renoir.op.net (root@renoir.op.net [209.152.193.4])
by candle.pha.pa.us (8.9.0/8.9.0) with ESMTP id IAA24423
for <maillist@candle.pha.pa.us>; Thu, 26 Nov 1998 08:31:08 -0500 (EST)
Received: from hub.org (majordom@hub.org [209.47.148.200]) by renoir.op.net (o1/$ Revision: 1.18 $) with ESMTP id IAA04554 for <maillist@candle.pha.pa.us>; Thu, 26 Nov 1998 08:04:30 -0500 (EST)
Received: from localhost (majordom@localhost)
by hub.org (8.9.1/8.9.1) with SMTP id HAA03761;
Thu, 26 Nov 1998 07:56:37 -0500 (EST)
(envelope-from owner-pgsql-hackers@hub.org)
Received: by hub.org (TLB v0.10a (1.23 tibbs 1997/01/09 00:29:32)); Thu, 26 Nov 1998 07:55:28 +0000 (EST)
Received: (from majordom@localhost)
by hub.org (8.9.1/8.9.1) id HAA03689
for pgsql-hackers-outgoing; Thu, 26 Nov 1998 07:55:26 -0500 (EST)
(envelope-from owner-pgsql-hackers@postgreSQL.org)
Received: from orion.SAPserv.Hamburg.dsh.de (Tpolaris2.sapham.debis.de [53.2.131.8])
by hub.org (8.9.1/8.9.1) with SMTP id HAA03674
for <pgsql-hackers@postgreSQL.org>; Thu, 26 Nov 1998 07:55:19 -0500 (EST)
(envelope-from wieck@sapserv.debis.de)
Received: by orion.SAPserv.Hamburg.dsh.de
for pgsql-hackers@postgreSQL.org
id m0zj13G-000EBfC; Thu, 26 Nov 98 14:01 MET
Message-Id: <m0zj13G-000EBfC@orion.SAPserv.Hamburg.dsh.de>
From: jwieck@debis.com (Jan Wieck)
Subject: Re: [HACKERS] Re: memory leak with Abort Transaction
To: takehi-s@ascii.co.jp (SHIOZAKI Takehiko)
Date: Thu, 26 Nov 1998 14:01:42 +0100 (MET)
Cc: pgsql-hackers@postgreSQL.org
Reply-To: jwieck@debis.com (Jan Wieck)
In-Reply-To: <199811261240.VAA27516@libpc01.pb.ascii.co.jp> from "SHIOZAKI Takehiko" at Nov 26, 98 09:40:19 pm
X-Mailer: ELM [version 2.4 PL25]
Content-Type: text
Sender: owner-pgsql-hackers@postgreSQL.org
Precedence: bulk
Status: RO
SHIOZAKI Takehiko wrote:
>
> Hello!
>
> Releasing 6.4.1 is a good news.
> But would you confirm the following "memory leak" problem?
> It is reproducable on 6.4 (FreeBSD 2.2.7-RELEASE).
It's an far too old problem. And as far as I remember, there
are different locations in the code causing it.
One place I remember well. It's in the tcop mainloop in
PostgresMain(). The querytree list is malloc()'ed (there and
in the parser) and free()'d after the query is processed -
except the processing of the queries bails out with elog().
In that case it never runs over the free() because the
longjmp() kick's it back to the beginning of the loop.
Jan
--
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me. #
#======================================== jwieck@debis.com (Jan Wieck) #
From owner-pgsql-hackers@hub.org Fri Mar 19 16:01:29 1999
Received: from hub.org (majordom@hub.org [209.47.145.100])
by candle.pha.pa.us (8.9.0/8.9.0) with ESMTP id QAA05828
for <maillist@candle.pha.pa.us>; Fri, 19 Mar 1999 16:01:22 -0500 (EST)
Received: from localhost (majordom@localhost)
by hub.org (8.9.2/8.9.1) with SMTP id PAA15701;
Fri, 19 Mar 1999 15:59:51 -0500 (EST)
(envelope-from owner-pgsql-hackers@hub.org)
Received: by hub.org (TLB v0.10a (1.23 tibbs 1997/01/09 00:29:32)); Fri, 19 Mar 1999 15:59:08 +0000 (EST)
Received: (from majordom@localhost)
by hub.org (8.9.2/8.9.1) id PAA15551
for pgsql-hackers-outgoing; Fri, 19 Mar 1999 15:59:05 -0500 (EST)
(envelope-from owner-pgsql-hackers@postgreSQL.org)
Received: from andrew.cmu.edu (ANDREW.CMU.EDU [128.2.10.101])
by hub.org (8.9.2/8.9.1) with ESMTP id PAA15524
for <pgsql-hackers@postgresql.org>; Fri, 19 Mar 1999 15:58:53 -0500 (EST)
(envelope-from er1p+@andrew.cmu.edu)
Received: (from postman@localhost) by andrew.cmu.edu (8.8.5/8.8.2) id PAA29323 for pgsql-hackers@postgresql.org; Fri, 19 Mar 1999 15:58:50 -0500 (EST)
Received: via switchmail; Fri, 19 Mar 1999 15:58:50 -0500 (EST)
Received: from cloudy.me.cmu.edu via qmail
ID </afs/andrew.cmu.edu/service/mailqs/q005/QF.cqwfdxK00gNtE0TVBp>;
Fri, 19 Mar 1999 15:58:37 -0500 (EST)
Received: from cloudy.me.cmu.edu via qmail
ID </afs/andrew.cmu.edu/usr2/er1p/.Outgoing/QF.MqwfdrO00gNtEmTPh2>;
Fri, 19 Mar 1999 15:58:31 -0500 (EST)
Received: from mms.4.60.Jun.27.1996.03.05.56.sun4.41.EzMail.2.0.CUILIB.3.45.SNAP.NOT.LINKED.cloudy.me.cmu.edu.sun4m.412
via MS.5.6.cloudy.me.cmu.edu.sun4_41;
Fri, 19 Mar 1999 15:58:29 -0500 (EST)
Message-ID: <wqwfdpu00gNtImTPUm@andrew.cmu.edu>
Date: Fri, 19 Mar 1999 15:58:29 -0500 (EST)
From: Erik Riedel <riedel+@CMU.EDU>
To: pgsql-hackers@postgreSQL.org
Subject: [HACKERS] aggregation memory leak and fix
Sender: owner-pgsql-hackers@postgreSQL.org
Precedence: bulk
Status: ROr
Platform: Alpha, Digital UNIX 4.0D
Software: PostgreSQL 6.4.2 and 6.5 snaphot (11 March 1999)
I have a table as follows:
Table = lineitem
+------------------------+----------------------------------+-------+
| Field | Type | Length|
+------------------------+----------------------------------+-------+
| l_orderkey | int4 not null | 4 |
| l_partkey | int4 not null | 4 |
| l_suppkey | int4 not null | 4 |
| l_linenumber | int4 not null | 4 |
| l_quantity | float4 not null | 4 |
| l_extendedprice | float4 not null | 4 |
| l_discount | float4 not null | 4 |
| l_tax | float4 not null | 4 |
| l_returnflag | char() not null | 1 |
| l_linestatus | char() not null | 1 |
| l_shipdate | date | 4 |
| l_commitdate | date | 4 |
| l_receiptdate | date | 4 |
| l_shipinstruct | char() not null | 25 |
| l_shipmode | char() not null | 10 |
| l_comment | char() not null | 44 |
+------------------------+----------------------------------+-------+
Index: lineitem_index_
that ends up having on the order of 500,000 rows (about 100 MB on disk).
I then run an aggregation query as:
--
-- Query 1
--
select l_returnflag, l_linestatus, sum(l_quantity) as sum_qty,
sum(l_extendedprice) as sum_base_price,
sum(l_extendedprice*(1-l_discount)) as sum_disc_price,
sum(l_extendedprice*(1-l_discount)*(1+l_tax)) as sum_charge,
avg(l_quantity) as avg_qty, avg(l_extendedprice) as avg_price,
avg(l_discount) as avg_disc, count(*) as count_order
from lineitem
where l_shipdate <= ('1998-12-01'::datetime - interval '90 day')::date
group by l_returnflag, l_linestatus
order by l_returnflag, l_linestatus;
when I run this against 6.4.2, the postgres process grows to upwards of
1 GB of memory (at which point something overflows and it dumps core) -
I watch it grow through 200 MB, 400 MB, 800 MB, dies somewhere near 1 GB
of allocated memory).
If I take out a few of the "sum" expressions it gets better, removing
sum_disk_price and sum_charge causes it to be only 600 MB and the query
actually (eventually) completes. Takes about 10 minutes on my 500 MHz
machine with 256 MB core and 4 GB of swap.
The problem seems to be the memory allocation mechanism. Looking at a
call trace, it is doing some kind of "sub query" plan for each row in
the database. That means it does ExecEval and postquel_function and
postquel_execute and all their friends for each row in the database.
Allocating a couple hundred bytes for each one.
The problem is that none of these allocations are freed - they seem to
depend on the AllocSet to free them at the end of the transaction. This
means it isn't a "true" leak, because the bytes are all freed at the
(very) end of the transaction, but it does mean that the process grows
to unreasonable size in the meantime. There is no need for this,
because the individual expression results are aggregated as it goes
along, so the intermediate nodes can be freed.
I spent half a day last week chasing down the offending palloc() calls
and execution stacks sufficiently that I think I found the right places
to put pfree() calls.
As a result, I have changes in the files:
src/backend/executor/execUtils.c
src/backend/executor/nodeResult.c
src/backend/executor/nodeAgg.c
src/backend/executor/execMain.c
patches to these files are attached at the end of this message. These
files are based on the 6.5.0 snapshot downloaded from ftp.postgreql.org
on 11 March 1999.
Apologies for sending patches to a non-released version. If anyone has
problems applying the patches, I can send the full files (I wanted to
avoid sending a 100K shell archive to the list). If anyone cares about
reproducing my exact problem with the above table, I can provide the 100
MB pg_dump file for download as well.
Secondary Issue: the reason I did not use the 6.4.2 code to make my
changes is because the AllocSet calls in that one were particularly
egregious - they only had the skeleton of the allocsets code that exists
in the 6.5 snapshots, so they were calling malloc() for all of the 8 and
16 byte allocations that the above query causes.
Using the fixed code reduces the maximum memory requirement on the above
query to about 210 MB, and reduces the runtime to (an acceptable) 1.5
minutes - a factor of more than 6x improvement on my 256 MB machine.
Now the biggest part of the execution time is in the sort before the
aggregation (which isn't strictly needed, but that is an optimization
for another day).
Open Issue: there is still a small "leak" that I couldn't eliminate, I
think I chased it down to the constvalue allocated in
execQual::ExecTargetList(), but I couldn't figure out where to properly
free it. 8 bytes leaked was much better than 750 bytes, so I stopped
banging my head on that particular item.
Secondary Open Issue: what I did have to do to get down to 210 MB of
core was reduce the minimum allocation size in AllocSet to 8 bytes from
16 bytes. That reduces the 8 byte leak above to a true 8 byte, rather
than a 16 byte leak. Otherwise, I think the size was 280 MB (still a
big improvement on 1000+ MB). I only changed this in my code and I am
not including a changed mcxt.c for that.
I hope my changes are understandable/reasonable. Enjoy.
Erik Riedel
Carnegie Mellon University
www.cs.cmu.edu/~riedel
--------------[aggregation_memory_patch.sh]-----------------------
#! /bin/sh
# This is a shell archive, meaning:
# 1. Remove everything above the #! /bin/sh line.
# 2. Save the resulting text in a file.
# 3. Execute the file with /bin/sh (not csh) to create:
# execMain.c.diff
# execUtils.c.diff
# nodeAgg.c.diff
# nodeResult.c.diff
# This archive created: Fri Mar 19 15:47:17 1999
export PATH; PATH=/bin:/usr/bin:$PATH
if test -f 'execMain.c.diff'
then
echo shar: "will not over-write existing file 'execMain.c.diff'"
else
cat << \SHAR_EOF > 'execMain.c.diff'
583c
.
398a
.
396a
/* XXX - clean up some more from ExecutorStart() - er1p */
if (NULL == estate->es_snapshot) {
/* nothing to free */
} else {
if (estate->es_snapshot->xcnt > 0) {
pfree(estate->es_snapshot->xip);
}
pfree(estate->es_snapshot);
}
if (NULL == estate->es_param_exec_vals) {
/* nothing to free */
} else {
pfree(estate->es_param_exec_vals);
estate->es_param_exec_vals = NULL;
}
.
SHAR_EOF
fi
if test -f 'execUtils.c.diff'
then
echo shar: "will not over-write existing file 'execUtils.c.diff'"
else
cat << \SHAR_EOF > 'execUtils.c.diff'
368a
}
/* ----------------
* ExecFreeExprContext
* ----------------
*/
void
ExecFreeExprContext(CommonState *commonstate)
{
ExprContext *econtext;
/* ----------------
* get expression context. if NULL then this node has
* none so we just return.
* ----------------
*/
econtext = commonstate->cs_ExprContext;
if (econtext == NULL)
return;
/* ----------------
* clean up memory used.
* ----------------
*/
pfree(econtext);
commonstate->cs_ExprContext = NULL;
}
/* ----------------
* ExecFreeTypeInfo
* ----------------
*/
void
ExecFreeTypeInfo(CommonState *commonstate)
{
TupleDesc tupDesc;
tupDesc = commonstate->cs_ResultTupleSlot->ttc_tupleDescriptor;
if (tupDesc == NULL)
return;
/* ----------------
* clean up memory used.
* ----------------
*/
FreeTupleDesc(tupDesc);
commonstate->cs_ResultTupleSlot->ttc_tupleDescriptor = NULL;
.
274a
.
SHAR_EOF
fi
if test -f 'nodeAgg.c.diff'
then
echo shar: "will not over-write existing file 'nodeAgg.c.diff'"
else
cat << \SHAR_EOF > 'nodeAgg.c.diff'
376a
pfree(oldVal); /* XXX - new, let's free the old datum - er1p */
.
374a
oldVal = value1[aggno]; /* XXX - save so we can free later - er1p */
.
112a
Datum oldVal = (Datum) NULL; /* XXX - so that we can save and free on
each iteration - er1p */
.
SHAR_EOF
fi
if test -f 'nodeResult.c.diff'
then
echo shar: "will not over-write existing file 'nodeResult.c.diff'"
else
cat << \SHAR_EOF > 'nodeResult.c.diff'
278a
pfree(resstate); node->resstate = NULL; /* XXX - new for us - er1p */
.
265a
ExecFreeExprContext(&resstate->cstate); /* XXX - new for us - er1p */
ExecFreeTypeInfo(&resstate->cstate); /* XXX - new for us - er1p */
.
SHAR_EOF
fi
exit 0
# End of shell archive
From er1p+@andrew.cmu.edu Fri Mar 19 19:43:27 1999
Received: from po8.andrew.cmu.edu (PO8.ANDREW.CMU.EDU [128.2.10.108])
by candle.pha.pa.us (8.9.0/8.9.0) with ESMTP id TAA09183
for <maillist@candle.pha.pa.us>; Fri, 19 Mar 1999 19:43:26 -0500 (EST)
Received: (from postman@localhost) by po8.andrew.cmu.edu (8.8.5/8.8.2) id TAA11773; Fri, 19 Mar 1999 19:43:18 -0500 (EST)
Received: via switchmail; Fri, 19 Mar 1999 19:43:18 -0500 (EST)
Received: from cloudy.me.cmu.edu via qmail
ID </afs/andrew.cmu.edu/service/mailqs/q007/QF.oqwiwLK00gNtQmTLgB>;
Fri, 19 Mar 1999 19:43:05 -0500 (EST)
Received: from mms.4.60.Jun.27.1996.03.05.56.sun4.41.EzMail.2.0.CUILIB.3.45.SNAP.NOT.LINKED.cloudy.me.cmu.edu.sun4m.412
via MS.5.6.cloudy.me.cmu.edu.sun4_41;
Fri, 19 Mar 1999 19:43:02 -0500 (EST)
Message-ID: <YqwiwKW00gNt4mTKsv@andrew.cmu.edu>
Date: Fri, 19 Mar 1999 19:43:02 -0500 (EST)
From: Erik Riedel <riedel+@CMU.EDU>
To: Bruce Momjian <maillist@candle.pha.pa.us>
Subject: Re: [HACKERS] aggregation memory leak and fix
Cc: pgsql-hackers@postgreSQL.org
In-Reply-To: <199903192223.RAA06691@candle.pha.pa.us>
References: <199903192223.RAA06691@candle.pha.pa.us>
Status: ROr
> No apologies necessary. Glad to have someone digging into that area of
> the code. We will gladly apply your patches to 6.5. However, I request
> that you send context diffs(diff -c). Normal diffs are just too
> error-prone in application. Send them, and I will apply them right
> away.
>
Context diffs attached. This was due to my ignorance of diff. When I
made the other files, I though "hmm, these could be difficult to apply
if the code has changed a bit, wouldn't it be good if they included a
few lines before and after the fix". Now I know "-c".
> Not sure why that is there? Perhaps for GROUP BY processing?
>
Right, it is a result of the Group processing requiring sorted input.
Just that it doesn't "require" sorted input, it "could" be a little more
flexible and the sort wouldn't be necessary. Essentially this would be
a single "AggSort" node that did the aggregation while sorting (probably
with replacement selection rather than quicksort). This definitely
would require some code/smarts that isn't there today.
> > think I chased it down to the constvalue allocated in
> > execQual::ExecTargetList(), but I couldn't figure out where to properly
> > free it. 8 bytes leaked was much better than 750 bytes, so I stopped
> > banging my head on that particular item.
>
> Can you give me the exact line? Is it the palloc(1)?
>
No, the 8 bytes seem to come from the ExecEvalExpr() call near line
1530. Problem was when I tried to free these, I got "not in AllocSet"
errors, so something more complicated was going on.
Thanks.
Erik
-----------[aggregation_memory_patch.sh]----------------------
#! /bin/sh
# This is a shell archive, meaning:
# 1. Remove everything above the #! /bin/sh line.
# 2. Save the resulting text in a file.
# 3. Execute the file with /bin/sh (not csh) to create:
# execMain.c.diff
# execUtils.c.diff
# nodeAgg.c.diff
# nodeResult.c.diff
# This archive created: Fri Mar 19 19:35:42 1999
export PATH; PATH=/bin:/usr/bin:$PATH
if test -f 'execMain.c.diff'
then
echo shar: "will not over-write existing file 'execMain.c.diff'"
else
cat << \SHAR_EOF > 'execMain.c.diff'
***
/afs/ece.cmu.edu/project/lcs/lcs-004/er1p/postgres/611/src/backend/executor/
execMain.c Thu Mar 11 23:59:11 1999
---
/afs/ece.cmu.edu/project/lcs/lcs-004/er1p/postgres/612/src/backend/executor/
execMain.c Fri Mar 19 15:03:28 1999
***************
*** 394,401 ****
--- 394,419 ----
EndPlan(queryDesc->plantree, estate);
+ /* XXX - clean up some more from ExecutorStart() - er1p */
+ if (NULL == estate->es_snapshot) {
+ /* nothing to free */
+ } else {
+ if (estate->es_snapshot->xcnt > 0) {
+ pfree(estate->es_snapshot->xip);
+ }
+ pfree(estate->es_snapshot);
+ }
+
+ if (NULL == estate->es_param_exec_vals) {
+ /* nothing to free */
+ } else {
+ pfree(estate->es_param_exec_vals);
+ estate->es_param_exec_vals = NULL;
+ }
+
/* restore saved refcounts. */
BufferRefCountRestore(estate->es_refcount);
+
}
void
***************
*** 580,586 ****
/*
* initialize result relation stuff
*/
!
if (resultRelation != 0 && operation != CMD_SELECT)
{
/*
--- 598,604 ----
/*
* initialize result relation stuff
*/
!
if (resultRelation != 0 && operation != CMD_SELECT)
{
/*
SHAR_EOF
fi
if test -f 'execUtils.c.diff'
then
echo shar: "will not over-write existing file 'execUtils.c.diff'"
else
cat << \SHAR_EOF > 'execUtils.c.diff'
***
/afs/ece.cmu.edu/project/lcs/lcs-004/er1p/postgres/611/src/backend/executor/
execUtils.c Thu Mar 11 23:59:11 1999
---
/afs/ece.cmu.edu/project/lcs/lcs-004/er1p/postgres/612/src/backend/executor/
execUtils.c Fri Mar 19 14:55:59 1999
***************
*** 272,277 ****
--- 272,278 ----
#endif
i++;
}
+
if (len > 0)
{
ExecAssignResultType(commonstate,
***************
*** 366,371 ****
--- 367,419 ----
pfree(projInfo);
commonstate->cs_ProjInfo = NULL;
+ }
+
+ /* ----------------
+ * ExecFreeExprContext
+ * ----------------
+ */
+ void
+ ExecFreeExprContext(CommonState *commonstate)
+ {
+ ExprContext *econtext;
+
+ /* ----------------
+ * get expression context. if NULL then this node has
+ * none so we just return.
+ * ----------------
+ */
+ econtext = commonstate->cs_ExprContext;
+ if (econtext == NULL)
+ return;
+
+ /* ----------------
+ * clean up memory used.
+ * ----------------
+ */
+ pfree(econtext);
+ commonstate->cs_ExprContext = NULL;
+ }
+
+ /* ----------------
+ * ExecFreeTypeInfo
+ * ----------------
+ */
+ void
+ ExecFreeTypeInfo(CommonState *commonstate)
+ {
+ TupleDesc tupDesc;
+
+ tupDesc = commonstate->cs_ResultTupleSlot->ttc_tupleDescriptor;
+ if (tupDesc == NULL)
+ return;
+
+ /* ----------------
+ * clean up memory used.
+ * ----------------
+ */
+ FreeTupleDesc(tupDesc);
+ commonstate->cs_ResultTupleSlot->ttc_tupleDescriptor = NULL;
}
/* ----------------------------------------------------------------
SHAR_EOF
fi
if test -f 'nodeAgg.c.diff'
then
echo shar: "will not over-write existing file 'nodeAgg.c.diff'"
else
cat << \SHAR_EOF > 'nodeAgg.c.diff'
***
/afs/ece.cmu.edu/project/lcs/lcs-004/er1p/postgres/611/src/backend/executor/
nodeAgg.c Thu Mar 11 23:59:11 1999
---
/afs/ece.cmu.edu/project/lcs/lcs-004/er1p/postgres/612/src/backend/executor/
nodeAgg.c Fri Mar 19 15:01:21 1999
***************
*** 110,115 ****
--- 110,116 ----
isNull2 = FALSE;
bool qual_result;
+ Datum oldVal = (Datum) NULL; /* XXX - so that we can save and free
on each iteration - er1p */
/* ---------------------
* get state info from node
***************
*** 372,379 ****
--- 373,382 ----
*/
args[0] = value1[aggno];
args[1] = newVal;
+ oldVal = value1[aggno]; /* XXX - save so we can free later - er1p */
value1[aggno] = (Datum) fmgr_c(&aggfns->xfn1,
(FmgrValues *) args, &isNull1);
+ pfree(oldVal); /* XXX - new, let's free the old datum - er1p */
Assert(!isNull1);
}
}
SHAR_EOF
fi
if test -f 'nodeResult.c.diff'
then
echo shar: "will not over-write existing file 'nodeResult.c.diff'"
else
cat << \SHAR_EOF > 'nodeResult.c.diff'
***
/afs/ece.cmu.edu/project/lcs/lcs-004/er1p/postgres/611/src/backend/executor/
nodeResult.c Thu Mar 11 23:59:12 1999
---
/afs/ece.cmu.edu/project/lcs/lcs-004/er1p/postgres/612/src/backend/executor/
nodeResult.c Fri Mar 19 14:57:26 1999
***************
*** 263,268 ****
--- 263,270 ----
* is freed at end-transaction time. -cim 6/2/91
* ----------------
*/
+ ExecFreeExprContext(&resstate->cstate); /* XXX - new for us - er1p */
+ ExecFreeTypeInfo(&resstate->cstate); /* XXX - new for us - er1p */
ExecFreeProjectionInfo(&resstate->cstate);
/* ----------------
***************
*** 276,281 ****
--- 278,284 ----
* ----------------
*/
ExecClearTuple(resstate->cstate.cs_ResultTupleSlot);
+ pfree(resstate); node->resstate = NULL; /* XXX - new for us - er1p */
}
void
SHAR_EOF
fi
exit 0
# End of shell archive
From owner-pgsql-hackers@hub.org Fri Mar 19 21:01:15 1999
Received: from hub.org (majordom@hub.org [209.47.145.100])
by candle.pha.pa.us (8.9.0/8.9.0) with ESMTP id VAA11368
for <maillist@candle.pha.pa.us>; Fri, 19 Mar 1999 21:01:13 -0500 (EST)
Received: from localhost (majordom@localhost)
by hub.org (8.9.2/8.9.1) with SMTP id UAA40887;
Fri, 19 Mar 1999 20:59:47 -0500 (EST)
(envelope-from owner-pgsql-hackers@hub.org)
Received: by hub.org (TLB v0.10a (1.23 tibbs 1997/01/09 00:29:32)); Fri, 19 Mar 1999 20:58:14 +0000 (EST)
Received: (from majordom@localhost)
by hub.org (8.9.2/8.9.1) id UAA40637
for pgsql-hackers-outgoing; Fri, 19 Mar 1999 20:58:12 -0500 (EST)
(envelope-from owner-pgsql-hackers@postgreSQL.org)
Received: from candle.pha.pa.us (maillist@s5-03.ppp.op.net [209.152.195.67])
by hub.org (8.9.2/8.9.1) with ESMTP id UAA40620
for <pgsql-hackers@postgreSQL.org>; Fri, 19 Mar 1999 20:58:02 -0500 (EST)
(envelope-from maillist@candle.pha.pa.us)
Received: (from maillist@localhost)
by candle.pha.pa.us (8.9.0/8.9.0) id UAA11263;
Fri, 19 Mar 1999 20:58:00 -0500 (EST)
From: Bruce Momjian <maillist@candle.pha.pa.us>
Message-Id: <199903200158.UAA11263@candle.pha.pa.us>
Subject: Re: [HACKERS] aggregation memory leak and fix
In-Reply-To: <YqwiwKW00gNt4mTKsv@andrew.cmu.edu> from Erik Riedel at "Mar 19, 1999 7:43: 2 pm"
To: riedel+@CMU.EDU (Erik Riedel)
Date: Fri, 19 Mar 1999 20:58:00 -0500 (EST)
Cc: pgsql-hackers@postgreSQL.org
X-Mailer: ELM [version 2.4ME+ PL47 (25)]
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: owner-pgsql-hackers@postgreSQL.org
Precedence: bulk
Status: ROr
>
> > No apologies necessary. Glad to have someone digging into that area of
> > the code. We will gladly apply your patches to 6.5. However, I request
> > that you send context diffs(diff -c). Normal diffs are just too
> > error-prone in application. Send them, and I will apply them right
> > away.
> >
> Context diffs attached. This was due to my ignorance of diff. When I
> made the other files, I though "hmm, these could be difficult to apply
> if the code has changed a bit, wouldn't it be good if they included a
> few lines before and after the fix". Now I know "-c".
Applied.
> > Not sure why that is there? Perhaps for GROUP BY processing?
> >
> Right, it is a result of the Group processing requiring sorted input.
> Just that it doesn't "require" sorted input, it "could" be a little more
> flexible and the sort wouldn't be necessary. Essentially this would be
> a single "AggSort" node that did the aggregation while sorting (probably
> with replacement selection rather than quicksort). This definitely
> would require some code/smarts that isn't there today.
I think you will find make_groupPlan adds the sort as needed by the
GROUP BY. I assume you are suggesting to do the aggregate/GROUP on unsorted
data, which is hard to do in a flexible way.
> > > think I chased it down to the constvalue allocated in
> > > execQual::ExecTargetList(), but I couldn't figure out where to properly
> > > free it. 8 bytes leaked was much better than 750 bytes, so I stopped
> > > banging my head on that particular item.
> >
> > Can you give me the exact line? Is it the palloc(1)?
> >
> No, the 8 bytes seem to come from the ExecEvalExpr() call near line
> 1530. Problem was when I tried to free these, I got "not in AllocSet"
> errors, so something more complicated was going on.
Yes, if you look inside ExecEvalExpr(), you will see it tries to get a
value for the expression(Datum). It may return an int, float4, or a
string. In the last case, that is actually a pointer and not a specific
value.
So, in some cases, the value can just be thrown away, or it may be a
pointer to memory that can be freed after the call to heap_formtuple()
later in the function. The trick is to find the function call in
ExecEvalExpr() that is allocating something, and conditionally free
values[] after the call to heap_formtuple(). If you don't want find it,
perhaps you can send me enough info so I can see it here.
I wonder whether it is the call to CreateTupleDescCopy() inside
ExecEvalVar()?
Another problem I just fixed is that fjIsNull was not being pfree'ed if
it was used with >64 targets, but I don't think that affects you.
I also assume you have run your recent patch through the the
test/regression tests, so see it does not cause some other area to fail,
right?
--
Bruce Momjian | http://www.op.net/~candle
maillist@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026
From owner-pgsql-hackers@hub.org Sat Mar 20 12:01:44 1999
Received: from renoir.op.net (root@renoir.op.net [209.152.193.4])
by candle.pha.pa.us (8.9.0/8.9.0) with ESMTP id MAA24855
for <maillist@candle.pha.pa.us>; Sat, 20 Mar 1999 12:01:43 -0500 (EST)
Received: from hub.org (majordom@hub.org [209.47.145.100]) by renoir.op.net (o1/$ Revision: 1.18 $) with ESMTP id LAA11985 for <maillist@candle.pha.pa.us>; Sat, 20 Mar 1999 11:58:48 -0500 (EST)
Received: from localhost (majordom@localhost)
by hub.org (8.9.2/8.9.1) with SMTP id LAA12367;
Sat, 20 Mar 1999 11:57:17 -0500 (EST)
(envelope-from owner-pgsql-hackers@hub.org)
Received: by hub.org (TLB v0.10a (1.23 tibbs 1997/01/09 00:29:32)); Sat, 20 Mar 1999 11:55:22 +0000 (EST)
Received: (from majordom@localhost)
by hub.org (8.9.2/8.9.1) id LAA12026
for pgsql-hackers-outgoing; Sat, 20 Mar 1999 11:55:17 -0500 (EST)
(envelope-from owner-pgsql-hackers@postgreSQL.org)
Received: from sss.sss.pgh.pa.us (sss.pgh.pa.us [206.210.65.6])
by hub.org (8.9.2/8.9.1) with ESMTP id LAA11871
for <pgsql-hackers@postgreSQL.org>; Sat, 20 Mar 1999 11:54:57 -0500 (EST)
(envelope-from tgl@sss.pgh.pa.us)
Received: from sss.sss.pgh.pa.us (localhost [127.0.0.1])
by sss.sss.pgh.pa.us (8.9.1/8.9.1) with ESMTP id LAA28068;
Sat, 20 Mar 1999 11:48:58 -0500 (EST)
To: Bruce Momjian <maillist@candle.pha.pa.us>
cc: riedel+@CMU.EDU (Erik Riedel), pgsql-hackers@postgreSQL.org
Subject: Re: [HACKERS] aggregation memory leak and fix
In-reply-to: Your message of Fri, 19 Mar 1999 21:33:33 -0500 (EST)
<199903200233.VAA11816@candle.pha.pa.us>
Date: Sat, 20 Mar 1999 11:48:58 -0500
Message-ID: <28066.921948538@sss.pgh.pa.us>
From: Tom Lane <tgl@sss.pgh.pa.us>
Sender: owner-pgsql-hackers@postgreSQL.org
Precedence: bulk
Status: ROr
Bruce Momjian <maillist@candle.pha.pa.us> writes:
> My only quick solution would seem to be to add a new "expression" memory
> context, that can be cleared after every tuple is processed, clearing
> out temporary values allocated inside an expression.
Right, this whole problem of growing backend memory use during a large
SELECT (or COPY, or probably a few other things) is one of the things
that we were talking about addressing by revising the memory management
structure.
I think what we want inside the executor is a distinction between
storage that must live to the end of the statement and storage that is
only needed while processing the current tuple. The second kind of
storage would go into a separate context that gets flushed every so
often. (It could be every tuple, or every dozen or hundred tuples
depending on what seems the best tradeoff of cycles against memory
usage.)
I'm not sure that just two contexts is enough, either. For example in
SELECT field1, SUM(field2) GROUP BY field1;
the working memory for the SUM aggregate could not be released after
each tuple, but perhaps we don't want it to live for the whole statement
either --- in that case we'd need a per-group context. (This particular
example isn't very convincing, because the same storage for the SUM
*could* be recycled from group to group. But I don't know whether it
actually *is* reused or not. If fresh storage is palloc'd for each
instantiation of SUM then we have a per-group leak in this scenario.
In any case, I'm not sure all aggregate functions have constant memory
requirements that would let them recycle storage across groups.)
What we need to do is work out what the best set of memory context
definitions is, and then decide on a strategy for making sure that
lower-level routines allocate their return values in the right context.
It'd be nice if the lower-level routines could still call palloc() and
not have to worry about this explicitly --- otherwise we'll break not
only a lot of our own code but perhaps a lot of user code. (User-
specific data types and SPI code all use palloc, no?)
I think it is too late to try to fix this for 6.5, but it ought to be a
top priority for 6.6.
regards, tom lane
From tgl@sss.pgh.pa.us Sun Mar 21 16:01:46 1999
Received: from renoir.op.net (root@renoir.op.net [209.152.193.4])
by candle.pha.pa.us (8.9.0/8.9.0) with ESMTP id QAA00139
for <maillist@candle.pha.pa.us>; Sun, 21 Mar 1999 16:01:45 -0500 (EST)
Received: from sss.sss.pgh.pa.us (sss.pgh.pa.us [206.210.65.6]) by renoir.op.net (o1/$ Revision: 1.18 $) with ESMTP id PAA27737 for <maillist@candle.pha.pa.us>; Sun, 21 Mar 1999 15:52:38 -0500 (EST)
Received: from sss.sss.pgh.pa.us (localhost [127.0.0.1])
by sss.sss.pgh.pa.us (8.9.1/8.9.1) with ESMTP id PAA14946;
Sun, 21 Mar 1999 15:50:20 -0500 (EST)
To: Bruce Momjian <maillist@candle.pha.pa.us>
cc: pgsql-hackers@postgreSQL.org
Subject: Re: [HACKERS] aggregation memory leak and fix
In-reply-to: Your message of Sun, 21 Mar 1999 14:20:39 -0500 (EST)
<199903211920.OAA28744@candle.pha.pa.us>
Date: Sun, 21 Mar 1999 15:50:20 -0500
Message-ID: <14944.922049420@sss.pgh.pa.us>
From: Tom Lane <tgl@sss.pgh.pa.us>
Status: ROr
Bruce Momjian <maillist@candle.pha.pa.us> writes:
>> What we need to do is work out what the best set of memory context
>> definitions is, and then decide on a strategy for making sure that
>> lower-level routines allocate their return values in the right context.
> Let's suppose that we want to free all the memory used as expression
> intermediate values after each row is processed.
> It is my understanding that all these are created in utils/adt/*.c
> files, and that the entry point to all those functions via
> fmgr()/fmgr_c().
That's probably the bulk of the specific calls of palloc(). Someone
(Jan?) did a scan of the code a while ago looking for palloc() calls,
and there aren't that many outside of the data-type-specific functions.
But we'd have to look individually at all the ones that are elsewhere.
> So, if we go into an expression memory context before calling
> fmgr/fmgr_c in the executor, and return to the normal context after the
> function call, all our intermediates are trapped in the expression
> memory context.
OK, so you're saying we leave the data-type-specific functions as is
(calling palloc() to allocate their result areas), and make each call
site specifically responsible for setting the context that palloc() will
allocate from? That could work, I think. We'd need to see what side
effects it'd have on other uses of palloc().
What we'd probably want is to use a stack discipline for the current
palloc-target memory context: when you set the context, you get back the
ID of the old context, and you are supposed to restore that old context
before returning.
> At the end of each row, we just free the expression memory context. In
> almost all cases, the data is stored in tuples, and we can free it. In
> a few cases like aggregates, we have to save off the value we need to
> keep before freeing the expression context.
Actually, nodeAgg would just have to set an appropriate context before
calling fmgr to execute the aggregate's transition functions, and then
it wouldn't need an extra copy step. The results would come back in the
right context already.
> In fact, you could even optimize the cleanup to only do free'ing if
> some expression memory was allocated. In most cases, it is not.
Jan's stuff should already fall through pretty quickly if there's
nothing in the context, I think. Note that what we want to do between
tuples is a "context clear" of the expression context, not a "context
delete" and then "context create" a new expression context. Context
clear should be a pretty quick no-op if nothing's been allocated in that
context...
> In fact the nodeAgg.c patch that I backed out attempted to do that,
> though because there wasn't code that checked if the Datum was
> pg_type.typbyval, it didn't work 100%.
Right. But if we approach it this way (clear the context at appropriate
times) rather than thinking in terms of explicitly pfree'ing individual
objects, life gets much simpler. Also, if we insist on being able to
pfree individual objects inside a context, we can't use Jan's faster
allocator! Remember, the reason it is faster and lower overhead is that
it doesn't keep track of individual objects, only pools.
I'd like to see us head in the direction of removing most of the
explicit pfree calls that exist now, and instead rely on clearing
memory contexts at appropriate times in order to manage memory.
The fewer places where we need pfree, the more contexts can be run
with the low-overhead space allocator. Also, the fewer explicit
pfrees we need, the simpler and more reliable the code gets.
regards, tom lane
From owner-pgsql-hackers@hub.org Sun Mar 21 16:01:49 1999
Received: from renoir.op.net (root@renoir.op.net [209.152.193.4])
by candle.pha.pa.us (8.9.0/8.9.0) with ESMTP id QAA00149
for <maillist@candle.pha.pa.us>; Sun, 21 Mar 1999 16:01:48 -0500 (EST)
Received: from hub.org (majordom@hub.org [209.47.145.100]) by renoir.op.net (o1/$ Revision: 1.18 $) with ESMTP id PAA27950 for <maillist@candle.pha.pa.us>; Sun, 21 Mar 1999 15:56:07 -0500 (EST)
Received: from localhost (majordom@localhost)
by hub.org (8.9.2/8.9.1) with SMTP id PAA39413;
Sun, 21 Mar 1999 15:54:51 -0500 (EST)
(envelope-from owner-pgsql-hackers@hub.org)
Received: by hub.org (TLB v0.10a (1.23 tibbs 1997/01/09 00:29:32)); Sun, 21 Mar 1999 15:54:31 +0000 (EST)
Received: (from majordom@localhost)
by hub.org (8.9.2/8.9.1) id PAA39249
for pgsql-hackers-outgoing; Sun, 21 Mar 1999 15:54:27 -0500 (EST)
(envelope-from owner-pgsql-hackers@postgreSQL.org)
Received: from sss.sss.pgh.pa.us (sss.pgh.pa.us [206.210.65.6])
by hub.org (8.9.2/8.9.1) with ESMTP id PAA39235
for <pgsql-hackers@postgreSQL.org>; Sun, 21 Mar 1999 15:54:21 -0500 (EST)
(envelope-from tgl@sss.pgh.pa.us)
Received: from sss.sss.pgh.pa.us (localhost [127.0.0.1])
by sss.sss.pgh.pa.us (8.9.1/8.9.1) with ESMTP id PAA14946;
Sun, 21 Mar 1999 15:50:20 -0500 (EST)
To: Bruce Momjian <maillist@candle.pha.pa.us>
cc: pgsql-hackers@postgreSQL.org
Subject: Re: [HACKERS] aggregation memory leak and fix
In-reply-to: Your message of Sun, 21 Mar 1999 14:20:39 -0500 (EST)
<199903211920.OAA28744@candle.pha.pa.us>
Date: Sun, 21 Mar 1999 15:50:20 -0500
Message-ID: <14944.922049420@sss.pgh.pa.us>
From: Tom Lane <tgl@sss.pgh.pa.us>
Sender: owner-pgsql-hackers@postgreSQL.org
Precedence: bulk
Status: RO
Bruce Momjian <maillist@candle.pha.pa.us> writes:
>> What we need to do is work out what the best set of memory context
>> definitions is, and then decide on a strategy for making sure that
>> lower-level routines allocate their return values in the right context.
> Let's suppose that we want to free all the memory used as expression
> intermediate values after each row is processed.
> It is my understanding that all these are created in utils/adt/*.c
> files, and that the entry point to all those functions via
> fmgr()/fmgr_c().
That's probably the bulk of the specific calls of palloc(). Someone
(Jan?) did a scan of the code a while ago looking for palloc() calls,
and there aren't that many outside of the data-type-specific functions.
But we'd have to look individually at all the ones that are elsewhere.
> So, if we go into an expression memory context before calling
> fmgr/fmgr_c in the executor, and return to the normal context after the
> function call, all our intermediates are trapped in the expression
> memory context.
OK, so you're saying we leave the data-type-specific functions as is
(calling palloc() to allocate their result areas), and make each call
site specifically responsible for setting the context that palloc() will
allocate from? That could work, I think. We'd need to see what side
effects it'd have on other uses of palloc().
What we'd probably want is to use a stack discipline for the current
palloc-target memory context: when you set the context, you get back the
ID of the old context, and you are supposed to restore that old context
before returning.
> At the end of each row, we just free the expression memory context. In
> almost all cases, the data is stored in tuples, and we can free it. In
> a few cases like aggregates, we have to save off the value we need to
> keep before freeing the expression context.
Actually, nodeAgg would just have to set an appropriate context before
calling fmgr to execute the aggregate's transition functions, and then
it wouldn't need an extra copy step. The results would come back in the
right context already.
> In fact, you could even optimize the cleanup to only do free'ing if
> some expression memory was allocated. In most cases, it is not.
Jan's stuff should already fall through pretty quickly if there's
nothing in the context, I think. Note that what we want to do between
tuples is a "context clear" of the expression context, not a "context
delete" and then "context create" a new expression context. Context
clear should be a pretty quick no-op if nothing's been allocated in that
context...
> In fact the nodeAgg.c patch that I backed out attempted to do that,
> though because there wasn't code that checked if the Datum was
> pg_type.typbyval, it didn't work 100%.
Right. But if we approach it this way (clear the context at appropriate
times) rather than thinking in terms of explicitly pfree'ing individual
objects, life gets much simpler. Also, if we insist on being able to
pfree individual objects inside a context, we can't use Jan's faster
allocator! Remember, the reason it is faster and lower overhead is that
it doesn't keep track of individual objects, only pools.
I'd like to see us head in the direction of removing most of the
explicit pfree calls that exist now, and instead rely on clearing
memory contexts at appropriate times in order to manage memory.
The fewer places where we need pfree, the more contexts can be run
with the low-overhead space allocator. Also, the fewer explicit
pfrees we need, the simpler and more reliable the code gets.
regards, tom lane
From owner-pgsql-hackers@hub.org Wed Mar 24 19:10:53 1999
Received: from renoir.op.net (root@renoir.op.net [209.152.193.4])
by candle.pha.pa.us (8.9.0/8.9.0) with ESMTP id TAA00906
for <maillist@candle.pha.pa.us>; Wed, 24 Mar 1999 19:10:52 -0500 (EST)
Received: from hub.org (majordom@hub.org [209.47.145.100]) by renoir.op.net (o1/$ Revision: 1.18 $) with ESMTP id NAA24258 for <maillist@candle.pha.pa.us>; Wed, 24 Mar 1999 13:09:47 -0500 (EST)
Received: from localhost (majordom@localhost)
by hub.org (8.9.2/8.9.1) with SMTP id NAA60743;
Wed, 24 Mar 1999 13:07:26 -0500 (EST)
(envelope-from owner-pgsql-hackers@hub.org)
Received: by hub.org (TLB v0.10a (1.23 tibbs 1997/01/09 00:29:32)); Wed, 24 Mar 1999 13:06:47 +0000 (EST)
Received: (from majordom@localhost)
by hub.org (8.9.2/8.9.1) id NAA60556
for pgsql-hackers-outgoing; Wed, 24 Mar 1999 13:06:43 -0500 (EST)
(envelope-from owner-pgsql-hackers@postgreSQL.org)
Received: from po7.andrew.cmu.edu (PO7.ANDREW.CMU.EDU [128.2.10.107])
by hub.org (8.9.2/8.9.1) with ESMTP id NAA60540
for <pgsql-hackers@postgreSQL.org>; Wed, 24 Mar 1999 13:06:25 -0500 (EST)
(envelope-from er1p+@andrew.cmu.edu)
Received: (from postman@localhost) by po7.andrew.cmu.edu (8.8.5/8.8.2) id NAA06323; Wed, 24 Mar 1999 13:06:16 -0500 (EST)
Received: via switchmail; Wed, 24 Mar 1999 13:06:16 -0500 (EST)
Received: from cloudy.me.cmu.edu via qmail
ID </afs/andrew.cmu.edu/service/mailqs/q001/QF.cqyGa::00gNtI0TZtD>;
Wed, 24 Mar 1999 13:06:02 -0500 (EST)
Received: from cloudy.me.cmu.edu via qmail
ID </afs/andrew.cmu.edu/usr2/er1p/.Outgoing/QF.sqyGa8G00gNtImTGBe>;
Wed, 24 Mar 1999 13:06:00 -0500 (EST)
Received: from mms.4.60.Jun.27.1996.03.05.56.sun4.41.EzMail.2.0.CUILIB.3.45.SNAP.NOT.LINKED.cloudy.me.cmu.edu.sun4m.412
via MS.5.6.cloudy.me.cmu.edu.sun4_41;
Wed, 24 Mar 1999 13:05:58 -0500 (EST)
Message-ID: <QqyGa6600gNtMmTG1o@andrew.cmu.edu>
Date: Wed, 24 Mar 1999 13:05:58 -0500 (EST)
From: Erik Riedel <riedel+@CMU.EDU>
To: Bruce Momjian <maillist@candle.pha.pa.us>
Subject: Re: [HACKERS] aggregation memory leak and fix
Cc: pgsql-hackers@postgreSQL.org
In-Reply-To: <199903240611.BAA01206@candle.pha.pa.us>
References: <199903240611.BAA01206@candle.pha.pa.us>
Sender: owner-pgsql-hackers@postgreSQL.org
Precedence: bulk
Status: RO
> I am interested to see if it fixes the expression leak you saw. I have
> not committed this yet. I want to look at it some more.
>
I'm afraid that this doesn't seem to have any effect on my query.
Looking at your code, I think the problem is that most of the
allocations in my query are on the top part of the if statement that
you modified (i.e. the == SQLlanguageId part). Below is a snippet of
a trace from my query, with approximate line numbers for execQual.c
with your patch applied:
(execQual) language == SQLlanguageId (execQual.c:757)
(execQual) execute postquel_function (execQual.c:759)
(mcxt) MemoryContextAlloc 32 bytes in ** Blank Portal **-heap
(mcxt) MemoryContextAlloc 16 bytes in ** Blank Portal **-heap
(mcxt) MemoryContextAlloc 528 bytes in ** Blank Portal **-heap
(mcxt) MemoryContextAlloc 56 bytes in ** Blank Portal **-heap
(mcxt) MemoryContextAlloc 88 bytes in ** Blank Portal **-heap
(mcxt) MemoryContextAlloc 24 bytes in ** Blank Portal **-heap
(mcxt) MemoryContextAlloc 8 bytes in ** Blank Portal **-heap
(mcxt) MemoryContextAlloc 65 bytes in ** Blank Portal **-heap
(mcxt) MemoryContextAlloc 48 bytes in ** Blank Portal **-heap
(mcxt) MemoryContextAlloc 8 bytes in ** Blank Portal **-heap
(execQual) else clause NOT SQLlanguageId (execQual.c:822)
(execQual) install qual memory context (execQual.c:858)
(execQual) exit qual context (execQual.c:862)
(mcxt) MemoryContextAlloc 60 bytes in ** Blank Portal **-heap
(mcxt) MemoryContextFree in ** Blank Portal **-heap freed 16 bytes
(mcxt) MemoryContextFree in ** Blank Portal **-heap freed 64 bytes
(mcxt) MemoryContextFree in ** Blank Portal **-heap freed 64 bytes
(mcxt) MemoryContextFree in ** Blank Portal **-heap freed 528 bytes
(mcxt) MemoryContextFree in ** Blank Portal **-heap freed 16 bytes
(execQual) return from postquel_function (execQual.c:764)
(execQual) return from ExecEvalFuncArgs (execQual.c:792)
(execQual) else clause NOT SQLlanguageId (execQual.c:822)
(execQual) install qual memory context (execQual.c:858)
(execQual) exit qual context (execQual.c:862)
(mcxt) MemoryContextAlloc 108 bytes in ** Blank Portal **-heap
(mcxt) MemoryContextAlloc 108 bytes in ** Blank Portal **-heap
(mcxt) MemoryContextFree in ** Blank Portal **-heap freed 128 bytes
(execQual) else clause NOT SQLlanguageId (execQual.c:822)
(execQual) install qual memory context (execQual.c:858)
(mcxt) MemoryContextAlloc 8 bytes in <Qual manager>-heap
(execQual) exit qual context (execQual.c:862)
<pattern repeats>
(execQual) language == SQLlanguageId (execQual.c:757)
(execQual) execute postquel_function (execQual.c:759)
(mcxt) MemoryContextAlloc 32 bytes in ** Blank Portal **-heap
(mcxt) MemoryContextAlloc 16 bytes in ** Blank Portal **-heap
(mcxt) MemoryContextAlloc 528 bytes in ** Blank Portal **-heap
(mcxt) MemoryContextAlloc 56 bytes in ** Blank Portal **-heap
(mcxt) MemoryContextAlloc 88 bytes in ** Blank Portal **-heap
(mcxt) MemoryContextAlloc 24 bytes in ** Blank Portal **-heap
(mcxt) MemoryContextAlloc 8 bytes in ** Blank Portal **-heap
(mcxt) MemoryContextAlloc 65 bytes in ** Blank Portal **-heap
(mcxt) MemoryContextAlloc 48 bytes in ** Blank Portal **-heap
(mcxt) MemoryContextAlloc 8 bytes in ** Blank Portal **-heap
(execQual) else clause NOT SQLlanguageId (execQual.c:822)
(execQual) install qual memory context (execQual.c:858)
(execQual) exit qual context (execQual.c:862)
(mcxt) MemoryContextAlloc 60 bytes in ** Blank Portal **-heap
(mcxt) MemoryContextFree in ** Blank Portal **-heap freed 16 bytes
(mcxt) MemoryContextFree in ** Blank Portal **-heap freed 64 bytes
(mcxt) MemoryContextFree in ** Blank Portal **-heap freed 64 bytes
(mcxt) MemoryContextFree in ** Blank Portal **-heap freed 528 bytes
(mcxt) MemoryContextFree in ** Blank Portal **-heap freed 16 bytes
(execQual) return from postquel_function (execQual.c:764)
(execQual) return from ExecEvalFuncArgs (execQual.c:792)
(execQual) else clause NOT SQLlanguageId (execQual.c:822)
(execQual) install qual memory context (execQual.c:858)
(execQual) exit qual context (execQual.c:862)
(mcxt) MemoryContextAlloc 108 bytes in ** Blank Portal **-heap
(mcxt) MemoryContextAlloc 108 bytes in ** Blank Portal **-heap
(mcxt) MemoryContextFree in ** Blank Portal **-heap freed 128 bytes
(execQual) else clause NOT SQLlanguageId (execQual.c:822)
(execQual) install qual memory context (execQual.c:858)
(mcxt) MemoryContextAlloc 8 bytes in <Qual manager>-heap
(execQual) exit qual context (execQual.c:862)
the MemoryContext lines give the name of the portal where each
allocation is happening - you see that your Qual manager only captures
a very small number (one) of the allocations, the rest are in the
upper part of the if statement.
Note that I also placed a printf next to your EndPortalAllocMode() and
StartPortalAllocMode() fix in ExecQual() - I believe this is what is
supposed to clear the portal and free the memory - and that printf
never appears in the above trace.
Sorry if the trace is a little confusing, but I hope that it helps you
zero in.
Erik
From owner-pgsql-hackers@hub.org Sat May 15 23:13:50 1999
Received: from hub.org (hub.org [209.167.229.1])
by candle.pha.pa.us (8.9.0/8.9.0) with ESMTP id XAA29144
for <maillist@candle.pha.pa.us>; Sat, 15 May 1999 23:13:49 -0400 (EDT)
Received: from hub.org (hub.org [209.167.229.1])
by hub.org (8.9.3/8.9.3) with ESMTP id XAA25173;
Sat, 15 May 1999 23:11:03 -0400 (EDT)
(envelope-from owner-pgsql-hackers@hub.org)
Received: by hub.org (TLB v0.10a (1.23 tibbs 1997/01/09 00:29:32)); Sat, 15 May 1999 23:10:29 +0000 (EDT)
Received: (from majordom@localhost)
by hub.org (8.9.3/8.9.3) id XAA25111
for pgsql-hackers-outgoing; Sat, 15 May 1999 23:10:27 -0400 (EDT)
(envelope-from owner-pgsql-hackers@postgreSQL.org)
X-Authentication-Warning: hub.org: majordom set sender to owner-pgsql-hackers@postgreSQL.org using -f
Received: from sss.sss.pgh.pa.us (sss.pgh.pa.us [206.210.65.6])
by hub.org (8.9.3/8.9.3) with ESMTP id XAA25092
for <pgsql-hackers@postgreSQL.org>; Sat, 15 May 1999 23:10:22 -0400 (EDT)
(envelope-from tgl@sss.pgh.pa.us)
Received: from sss.sss.pgh.pa.us (localhost [127.0.0.1])
by sss.sss.pgh.pa.us (8.9.1/8.9.1) with ESMTP id XAA17752
for <pgsql-hackers@postgreSQL.org>; Sat, 15 May 1999 23:09:46 -0400 (EDT)
To: pgsql-hackers@postgreSQL.org
Subject: [HACKERS] Memory leaks in relcache
Date: Sat, 15 May 1999 23:09:46 -0400
Message-ID: <17750.926824186@sss.pgh.pa.us>
From: Tom Lane <tgl@sss.pgh.pa.us>
Sender: owner-pgsql-hackers@postgreSQL.org
Precedence: bulk
Status: ROr
I have been looking into why a reference to a nonexistent table, eg
INSERT INTO nosuchtable VALUES(1);
leaks a small amount of memory per occurrence. What I find is a
memory leak in the indexscan support. Specifically,
RelationGetIndexScan in backend/access/index/genam.c palloc's both
an IndexScanDesc and some keydata storage. The IndexScanDesc
block is eventually pfree'd, at the bottom of CatalogIndexFetchTuple
in backend/catalog/indexing.c. But the keydata block is not.
This wouldn't matter so much if the palloc were coming from a
transaction-local context. But what we're doing is a lookup in pg_class
on behalf of RelationBuildDesc in backend/utils/cache/relcache.c, and
it's done a MemoryContextSwitchTo into the global CacheCxt before
starting the lookup. Therefore, the un-pfreed block represents a
permanent memory leak.
In fact, *every* reference to a relation that is not already present in
the relcache causes a similar leak. The error case is just the one that
is easiest to repeat. The missing pfree of the keydata block is
probably causing a bunch of other short-term and long-term leaks too.
It seems to me there are two things to fix here: indexscan ought to
pfree everything it pallocs, and RelationBuildDesc ought to be warier
about how much work gets done with CacheCxt as the active palloc
context. (Even if indexscan didn't leak anything ordinarily, there's
still the risk of elog(ERROR) causing an abort before the indexscan code
gets to clean up.)
Comments? In particular, where is the cleanest place to add the pfree
of the keydata block? I don't especially like the fact that callers
of index_endscan have to clean up the toplevel scan block; I think that
ought to happen inside index_endscan.
regards, tom lane
src/backend/parser/analyze.c
View file @ 745f0c21
......@@ -6,7 +6,7 @@
* Portions Copyright (c) 1996-2000, PostgreSQL, Inc
* Portions Copyright (c) 1994, Regents of the University of California
*
* $Id: analyze.c,v 1.174 2001/01/05 06:34:18 tgl Exp $
* $Id: analyze.c,v 1.175 2001/01/08 20:54:24 momjian Exp $
*
*-------------------------------------------------------------------------
*/
......@@ -1140,7 +1140,7 @@ transformCreateStmt(ParseState *pstate, CreateStmt *stmt)
int found=0;
List *cols;
List *fkattrs;
Ident *fkattr;
Ident *fkattr = NULL;
ColumnDef *col;
foreach(fkattrs, fkconstraint->fk_attrs) {
found=0;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment