From: Alvaro Herrera <alvherre@commandprompt.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Nick <t32@2thebatcave.com>
Cc: Kevin Grittner <kevin.grittner@wicourts.gov>
Cc: pgsql-admin <pgsql-admin@postgresql.org>
Subject: Re: autovacuum launcher process eating up 17G+ of ram?
Date: Mon, 08 Nov 2010 12:01:51 -0300
Message-ID: <1289227352-sup-6579@alvh.no-ip.org> (raw)
In-Reply-To: <4232.1288036630@sss.pgh.pa.us>
References: <AANLkTimGvWugpjwN1Ki=qmd56nsc4gXH6yKT5765nFPK@mail.gmail.com>
<4CC071670200002500036C8A@gw.wicourts.gov>
<AANLkTimKs23hytr6oj1Kco53rbaPF2mKTZZYofr0Cr0+@mail.gmail.com>
<4CC18EF60200002500036CF3@gw.wicourts.gov>
<AANLkTi=Mh=pv7Ai+L17cbjgsExMbncHG5fMTvj5_wCAe@mail.gmail.com>
<4232.1288036630@sss.pgh.pa.us>
Excerpts from Tom Lane's message of lun oct 25 16:57:10 -0300 2010:
> It looks to me like the problem is that get_database_list()
> intentionally pushes its result into a long-lived context, and then the
> result is never cleaned up. It doesn't help any that it does a lot of
> leaky things like heap_beginscan with CurrentMemoryContext totally
> unnecessarily pointing at the long-lived context.
>
> I'm not sure what the intent was here, but it's pretty broken as-is.
> Alvaro, what did you have in mind? Should this be using a less
> long-lived context, or do we need code to free an avw_dbase list?
Sorry for not noticing this earlier. Seems my mail filters are broken
:-( Thanks Tom for the ping via private email.
Yes, get_database_list is clearly broken as is. I don't remember why
it's coded like this; this memcxt shuffling was introducing in the 9.0
rewrite to read pg_database directly instead of the flat file.
The right way for it to work seems to be to allocate the output list in
the caller's context; both current callers are in a throwaway context and
so the allocated list will go away automatically when they're done with
the list.
Patch for this is attached (applies cleanly to 9.0 and HEAD; 8.4 doesn't
need to be patched, but I'll run the test case now just to be sure.)
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Attachments:0001-Fix-permanent-memory-leak-in-autovacuum-launcher.patch (application/octet-stream, 2.8K) download
From c5c6170c2e9d4555f0fcb15afa6d7e47595572d0 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Mon, 8 Nov 2010 11:28:09 -0300
Subject: [PATCH] Fix permanent memory leak in autovacuum launcher
get_database_list was uselessly allocating its output data, along some
created along the way, in a permanent memory context. This didn't
matter when autovacuum was a single, short-lived process, but now that
the launcher is permanent, it shows up as a permanent leak.
To fix, make get_database list allocate its output data in the caller's
context, which is in charge of freeing it when appropriate; and the
memory leaked by heap_beginscan et al is allocated in a throwaway
transaction context.
---
src/backend/postmaster/autovacuum.c | 20 +++++++++++++++++---
1 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.cindex e717644..22cafd5 100644--- a/src/backend/postmaster/autovacuum.c+++ b/src/backend/postmaster/autovacuum.c@@ -1754,6 +1754,9 @@ autovac_balance_cost(void)
* get_database_list
* Return a list of all databases found in pg_database.
*
+ * The list and associated data is allocated in the caller's memory context,+ * which is in charge of ensuring that it's properly cleaned up afterwards.+ *
* Note: this is the only function in which the autovacuum launcher uses a
* transaction. Although we aren't attached to any particular database and
* therefore can't access most catalogs, we do have enough infrastructure
@@ -1766,6 +1769,10 @@ get_database_list(void)
Relation rel;
HeapScanDesc scan;
HeapTuple tup;
+ MemoryContext cxt;++ /* This is the context that we will allocate our output data in */+ cxt = CurrentMemoryContext;
/*
* Start a transaction so we can access pg_database, and get a snapshot.
@@ -1777,9 +1784,6 @@ get_database_list(void)
StartTransactionCommand();
(void) GetTransactionSnapshot();
- /* Allocate our results in AutovacMemCxt, not transaction context */- MemoryContextSwitchTo(AutovacMemCxt);-
rel = heap_open(DatabaseRelationId, AccessShareLock);
scan = heap_beginscan(rel, SnapshotNow, 0, NULL);
@@ -1787,6 +1791,15 @@ get_database_list(void)
{
Form_pg_database pgdatabase = (Form_pg_database) GETSTRUCT(tup);
avw_dbase *avdb;
+ MemoryContext oldcxt;++ /*+ * Allocate our results in the caller's context, not the transaction's.+ * We do this inside the loop, and restore the original context at the+ * end, so that leaky things like heap_getnext() are not called in a+ * potentially long-lived context.+ */+ oldcxt = MemoryContextSwitchTo(cxt);
avdb = (avw_dbase *) palloc(sizeof(avw_dbase));
@@ -1797,6 +1810,7 @@ get_database_list(void)
avdb->adw_entry = NULL;
dblist = lappend(dblist, avdb);
+ MemoryContextSwitchTo(oldcxt);
}
heap_endscan(scan);
--
1.7.1
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-admin@postgresql.org
Cc: alvherre@commandprompt.com, tgl@sss.pgh.pa.us, t32@2thebatcave.com, kevin.grittner@wicourts.gov
Subject: Re: autovacuum launcher process eating up 17G+ of ram?
In-Reply-To: <1289227352-sup-6579@alvh.no-ip.org>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox