Memory leak in deferrable index constraints
Oops, my fault. The list returned by ExecInsertIndexTuples() needs to be
freed otherwise lots of lists (one per row) will build up and not be freed
until the end of the query. This actually accounts for even more memory
than the after-trigger event queue. Patch attached.
Of course the after-trigger queue still uses a lot of memory for large
updates (I was working on a patch for that but ran out of time before
this commitfest started). This fix at least brings deferred index
constraints into line with FK constraints, in terms of memory usage.
Regards,
Dean
Attachments:
memory_leak.patchapplication/octet-stream; name=memory_leak.patchDownload
*** ./src/backend/commands/copy.c.orig 2010-01-23 10:48:36.000000000 +0000
--- ./src/backend/commands/copy.c 2010-01-23 10:49:20.000000000 +0000
***************
*** 2171,2176 ****
--- 2171,2178 ----
/* AFTER ROW INSERT Triggers */
ExecARInsertTriggers(estate, resultRelInfo, tuple,
recheckIndexes);
+ if (recheckIndexes != NIL)
+ list_free(recheckIndexes);
/*
* We count only tuples not suppressed by a BEFORE INSERT trigger;
*** ./src/backend/executor/nodeModifyTable.c.orig 2010-01-23 10:45:33.000000000 +0000
--- ./src/backend/executor/nodeModifyTable.c 2010-01-23 10:47:15.000000000 +0000
***************
*** 253,258 ****
--- 253,260 ----
/* AFTER ROW INSERT Triggers */
ExecARInsertTriggers(estate, resultRelInfo, tuple, recheckIndexes);
+ if (recheckIndexes != NIL)
+ list_free(recheckIndexes);
/* Process RETURNING if present */
if (resultRelInfo->ri_projectReturning)
***************
*** 569,574 ****
--- 571,578 ----
/* AFTER ROW UPDATE Triggers */
ExecARUpdateTriggers(estate, resultRelInfo, tupleid, tuple,
recheckIndexes);
+ if (recheckIndexes != NIL)
+ list_free(recheckIndexes);
/* Process RETURNING if present */
if (resultRelInfo->ri_projectReturning)
Dean Rasheed <dean.a.rasheed@googlemail.com> writes:
Oops, my fault. The list returned by ExecInsertIndexTuples() needs to be
freed otherwise lots of lists (one per row) will build up and not be freed
until the end of the query. This actually accounts for even more memory
than the after-trigger event queue. Patch attached.
It seems a bit unlikely that this would be the largest memory leak in
that area. Can you show a test case that demonstrates this is worth
worrying about?
regards, tom lane
On 31 January 2010 16:03, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Dean Rasheed <dean.a.rasheed@googlemail.com> writes:
Oops, my fault. The list returned by ExecInsertIndexTuples() needs to be
freed otherwise lots of lists (one per row) will build up and not be freed
until the end of the query. This actually accounts for even more memory
than the after-trigger event queue. Patch attached.It seems a bit unlikely that this would be the largest memory leak in
that area. Can you show a test case that demonstrates this is worth
worrying about?
If I do
create table foo(a int unique deferrable initially deferred);
insert into foo (select * from generate_series(1, 10000000));
begin;
update foo set a=a+1;
set constraints all immediate;
commit;
and watch the backend memory usage during the UPDATE, it grows to
around 970MB and then falls back to 370MB as soon as the UPDATE
command finishes. During whole SET CONSTRAINTS trigger execution
it then remains constant at 370MB.
With this patch, it never grows beyond the 370MB occupied by the
after-triggers queue.
Regards,
Dean
Dean Rasheed <dean.a.rasheed@googlemail.com> writes:
On 31 January 2010 16:03, Tom Lane <tgl@sss.pgh.pa.us> wrote:
It seems a bit unlikely that this would be the largest memory leak in
that area. �Can you show a test case that demonstrates this is worth
worrying about?
create table foo(a int unique deferrable initially deferred);
insert into foo (select * from generate_series(1, 10000000));
begin;
update foo set a=a+1;
set constraints all immediate;
commit;
Thanks. I had forgotten all the work we put into minimizing the size of
the deferred trigger queue. In this example it's only 16 bytes per
entry, whereas a 1-element List is going to involve 16 bytes for the
header, 8 bytes for the cell, plus two palloc item overheads --- and
double all that on a 64-bit machine. So yeah, this is a significant
leak. Patch applied.
regards, tom lane