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 ; 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 ; 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 ; 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: 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 ; 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 ; 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 ; Fri, 19 Mar 1999 15:58:37 -0500 (EST) Received: from cloudy.me.cmu.edu via qmail ID ; 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: Date: Fri, 19 Mar 1999 15:58:29 -0500 (EST) From: Erik Riedel 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 ; 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 ; 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: Date: Fri, 19 Mar 1999 19:43:02 -0500 (EST) From: Erik Riedel To: Bruce Momjian 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 ; 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 ; 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 Message-Id: <199903200158.UAA11263@candle.pha.pa.us> Subject: Re: [HACKERS] aggregation memory leak and fix In-Reply-To: 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 ; 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 ; 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 ; 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 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 Sender: owner-pgsql-hackers@postgreSQL.org Precedence: bulk Status: ROr Bruce Momjian 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 ; 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 ; 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 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 Status: ROr Bruce Momjian 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 ; 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 ; 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 ; 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 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 Sender: owner-pgsql-hackers@postgreSQL.org Precedence: bulk Status: RO Bruce Momjian 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 ; 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 ; 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 ; 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 ; Wed, 24 Mar 1999 13:06:02 -0500 (EST) Received: from cloudy.me.cmu.edu via qmail ID ; 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: Date: Wed, 24 Mar 1999 13:05:58 -0500 (EST) From: Erik Riedel To: Bruce Momjian 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 -heap (execQual) exit qual context (execQual.c:862) (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 -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 ; 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 ; 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 ; 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 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