Re: [HACKERS] aggregation memory leak and fix
От | Erik Riedel |
---|---|
Тема | Re: [HACKERS] aggregation memory leak and fix |
Дата | |
Msg-id | YqwiwKW00gNt4mTKsv@andrew.cmu.edu обсуждение исходный текст |
Ответ на | Re: [HACKERS] aggregation memory leak and fix (Bruce Momjian <maillist@candle.pha.pa.us>) |
Ответы |
Re: [HACKERS] aggregation memory leak and fix
(Bruce Momjian <maillist@candle.pha.pa.us>)
Re: [HACKERS] aggregation memory leak and fix (Bruce Momjian <maillist@candle.pha.pa.us>) Re: [HACKERS] aggregation memory leak and fix (Bruce Momjian <maillist@candle.pha.pa.us>) |
Список | pgsql-hackers |
> 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' thenecho 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' thenecho 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' thenecho 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' thenecho 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
В списке pgsql-hackers по дате отправления: