Обсуждение: Memory leak in FDW
Foreign data wrapper's IterateForeignScan() function is supposed to be called in a short-lived memory context, but the memory context is actually not reset during query execution. That's a pretty bad memory leak. I've been testing this with file_fdw and a large file, and "SELECT COUNT(*) FROM foreign_table" Interestingly, if you add any WHERE clause to it, the memory context is reset in ExecScan and the leak goes away. This is only a problem with the fastpath in ExecScan for the case of no quals and no projections. The trivial fix is to reset the per-tuple memory context between iterations. I tried to look around for other executor nodes that might have the same problem. I didn't see any obvious leaks, although index scan node seems to call AM's getnext without resetting the memory context in between. That's a pretty well-tested codepath, however, and there hasn't been any complains of leaks with index scans, so there must be something that mitigates it. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Вложения
Excerpts from Heikki Linnakangas's message of mar abr 26 15:06:51 -0300 2011: > I tried to look around for other executor nodes that might > have the same problem. I didn't see any obvious leaks, although index > scan node seems to call AM's getnext without resetting the memory > context in between. That's a pretty well-tested codepath, however, and > there hasn't been any complains of leaks with index scans, so there must > be something that mitigates it. Don't we have some rule that functions used in index AMs are supposed to be leak-free? -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes: > The trivial fix is to reset the per-tuple memory context between > iterations. Have you tested this with SRFs? ForeignNext seems like quite the wrong place for resetting exprcontext in any case ... regards, tom lane
On Tue, Apr 26, 2011 at 7:15 PM, Alvaro Herrera <alvherre@commandprompt.com> wrote: > Excerpts from Heikki Linnakangas's message of mar abr 26 15:06:51 -0300 2011: > >> I tried to look around for other executor nodes that might >> have the same problem. I didn't see any obvious leaks, although index >> scan node seems to call AM's getnext without resetting the memory >> context in between. That's a pretty well-tested codepath, however, and >> there hasn't been any complains of leaks with index scans, so there must >> be something that mitigates it. > > Don't we have some rule that functions used in index AMs are supposed to > be leak-free? btree operators and opclass functions are supposed to be leak-free. I think other AMs don't try to have the same strictness. -- greg
On 26.04.2011 21:30, Tom Lane wrote: > Heikki Linnakangas<heikki.linnakangas@enterprisedb.com> writes: >> The trivial fix is to reset the per-tuple memory context between >> iterations. > > Have you tested this with SRFs? > > ForeignNext seems like quite the wrong place for resetting > exprcontext in any case ... ExecScan would be more appropriate I guess (attached). This means the context will be reset between each tuple even for nodes like seqscan that don't use the per-tuple context for anything. AllocSetReset exits quickly if there's nothing to do, but it takes a couple of function calls to get there. I wouldn't normally worry about that, but this is a very hot path for simple queries. I tested this with: CREATE TABLE foo AS SELECT generate_series(1,10000000); I ran "SELECT COUNT(*) FROM foo" many times with \timing on, and took the smallest time with and without the patch. I got: 1230 ms with the patch 1186 ms without the patch This was quite repeatable, it's consistently faster without the patch. That's a bigger difference than I expected. Any random code change can swing results on micro-benchmarks like this by 1-2%, but that's over 3%. Do we care? I might be getting a bit carried away with this, but we could buy that back by moving the isReset flag from AllocSetContext to MemoryContextData. That way MemoryContextReset can exit more quickly if there's nothing to do, patch attached. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Вложения
On 27.04.2011 04:19, Heikki Linnakangas wrote: > On 26.04.2011 21:30, Tom Lane wrote: >> Heikki Linnakangas<heikki.linnakangas@enterprisedb.com> writes: >>> The trivial fix is to reset the per-tuple memory context between >>> iterations. >> >> Have you tested this with SRFs? >> >> ForeignNext seems like quite the wrong place for resetting >> exprcontext in any case ... > > ExecScan would be more appropriate I guess (attached). > > This means the context will be reset between each tuple even for nodes > like seqscan that don't use the per-tuple context for anything. > AllocSetReset exits quickly if there's nothing to do, but it takes a > couple of function calls to get there. I wouldn't normally worry about > that, but this is a very hot path for simple queries. > > I tested this with: > > CREATE TABLE foo AS SELECT generate_series(1,10000000); > > I ran "SELECT COUNT(*) FROM foo" many times with \timing on, and took > the smallest time with and without the patch. I got: > > 1230 ms with the patch > 1186 ms without the patch > > This was quite repeatable, it's consistently faster without the patch. > That's a bigger difference than I expected. Any random code change can > swing results on micro-benchmarks like this by 1-2%, but that's over 3%. > Do we care? > > I might be getting a bit carried away with this, but we could buy that > back by moving the isReset flag from AllocSetContext to > MemoryContextData. That way MemoryContextReset can exit more quickly if > there's nothing to do, patch attached. I hear no objections, so committed. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com