Thread: memory leaks? using savepoint
Hi! When I tested simple query as following, backend process used much memory and not freed until the backend was finished. # This is reproduced on PostgreSQL8.3 (PostgreSQL8.3.6 - PostgreSQL8.3.12) ------- BEGIN; SAVEPOINT sp0; INSERT INTO tbl VALUES (1); RELEASE sp0; SAVEPOINT sp1; INSERT INTO tbl VALUES (1); RELEASE sp1; (repeats 10k) ROLLBACK; SELECT pg_sleep(10000); ------- I think this caused by following fix. http://archives.postgresql.org/pgsql-committers/2008-12/msg00100.php http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/tcop/postgres.c?r1=1.560&r2=1.561) Should "CopySnapshot()" be performed in MessageContext ? Attached simple pathch to solve it... Best regards, -- NTT OSS Center Tatsuhito Kasahara kasahara.tatsuhito@oss.ntt.co.jp diff -rcN postgresql-8.3.12/src/backend/tcop/postgres.c postgresql-8.3.12_dev/src/backend/tcop/postgres.c *** postgresql-8.3.12/src/backend/tcop/postgres.c 2010-10-01 22:36:12.000000000 +0900 --- postgresql-8.3.12_dev/src/backend/tcop/postgres.c 2010-12-16 15:46:43.000000000 +0900 *************** *** 914,919 **** --- 914,921 ---- /* * Set up a snapshot if parse analysis/planning will need one. */ + oldcontext = MemoryContextSwitchTo(MessageContext); + if (analyze_requires_snapshot(parsetree)) { mySnapshot = CopySnapshot(GetTransactionSnapshot()); *************** *** 926,932 **** * Switch to appropriate context for constructing querytrees (again, * these must outlive the execution context). */ - oldcontext = MemoryContextSwitchTo(MessageContext); querytree_list = pg_analyze_and_rewrite(parsetree, query_string, NULL, 0); --- 928,933 ----
Tatsuhito Kasahara <kasahara.tatsuhito@oss.ntt.co.jp> writes:
> When I tested simple query as following, backend process used much memory
> and not freed until the backend was finished.
> # This is reproduced on PostgreSQL8.3 (PostgreSQL8.3.6 - PostgreSQL8.3.12)
Hmm ... this test case doesn't appear to produce any significant memory
leakage in 8.4 and up. What is happening in 8.3 is that the
CurTransactionContext of each subtransaction becomes nonempty, so it
eats 8K or so even though the snapshot gets released shortly later.
There are plenty of other ways to cause that to happen, though, so I'm
not particularly excited about fixing this one ... especially not in a
stable branch that's not getting a lot of developer testing anymore.
I'm inclined to leave this alone --- I think the risks of patching only
an old branch will outweigh the benefits.
regards, tom lane
On Fri, Dec 17, 2010 at 7:27 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Tatsuhito Kasahara <kasahara.tatsuhito@oss.ntt.co.jp> writes: >> When I tested simple query as following, backend process used much memory >> and not freed until the backend was finished. >> # This is reproduced on PostgreSQL8.3 (PostgreSQL8.3.6 - PostgreSQL8.3.1= 2) > > Hmm ... this test case doesn't appear to produce any significant memory > leakage in 8.4 and up. =A0What is happening in 8.3 is that the > CurTransactionContext of each subtransaction becomes nonempty, so it > eats 8K or so even though the snapshot gets released shortly later. > There are plenty of other ways to cause that to happen, though, so I'm > not particularly excited about fixing this one ... especially not in a > stable branch that's not getting a lot of developer testing anymore. > I'm inclined to leave this alone --- I think the risks of patching only > an old branch will outweigh the benefits. The proposed patch looks very simple. I don't think that applying that patch will cause serious risk. Unless the bug is fixed, the users who encountered the memory-leak cannot update their postgres to the latest version of 8.3. This would cause more serious situation. And, since psqlODBC frequently issues SAVEPONT and RELEASE SAVEPOINT, this memory-leak is not rare case, I think. Regards, --=20 Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
Fujii Masao <masao.fujii@gmail.com> writes:
> The proposed patch looks very simple. I don't think that applying that
> patch will cause serious risk.
Maybe so, maybe not, but *it won't get tested* to any meaningful degree
if it's applied.
> Unless the bug is fixed, the users who encountered the memory-leak
> cannot update their postgres to the latest version of 8.3. This would
> cause more serious situation. And, since psqlODBC frequently issues
> SAVEPONT and RELEASE SAVEPOINT, this memory-leak is not rare case,
> I think.
The code's been like that for two years, and nobody's complained before
now, so it doesn't seem to me to be all that urgent. Furthermore, the
people you are speaking of likely wouldn't upgrade anyway if they
haven't done so before now...
regards, tom lane
On Tue, Dec 21, 2010 at 9:26 PM, Fujii Masao <masao.fujii@gmail.com> wrote: > On Fri, Dec 17, 2010 at 7:27 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Tatsuhito Kasahara <kasahara.tatsuhito@oss.ntt.co.jp> writes: >>> When I tested simple query as following, backend process used much memo= ry >>> and not freed until the backend was finished. >>> # This is reproduced on PostgreSQL8.3 (PostgreSQL8.3.6 - PostgreSQL8.3.= 12) >> >> Hmm ... this test case doesn't appear to produce any significant memory >> leakage in 8.4 and up. =A0What is happening in 8.3 is that the >> CurTransactionContext of each subtransaction becomes nonempty, so it >> eats 8K or so even though the snapshot gets released shortly later. >> There are plenty of other ways to cause that to happen, though, so I'm >> not particularly excited about fixing this one ... especially not in a >> stable branch that's not getting a lot of developer testing anymore. >> I'm inclined to leave this alone --- I think the risks of patching only >> an old branch will outweigh the benefits. > > The proposed patch looks very simple. I don't think that applying that > patch will cause serious risk. > > Unless the bug is fixed, the users who encountered the memory-leak > cannot update their postgres to the latest version of 8.3. This would > cause more serious situation. And, since psqlODBC frequently issues > SAVEPONT and RELEASE SAVEPOINT, this memory-leak is not rare case, > I think. Are you saying that this problem does not exist in 8.3.0 but does exist in later 8.3.x revs? --=20 Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> Are you saying that this problem does not exist in 8.3.0 but does
> exist in later 8.3.x revs?
I believe it dates from
Author: Tom Lane <tgl@sss.pgh.pa.us>
Branch: master Release: REL8_4_BR [c98a92378] 2008-12-13 02:00:20 +0000
Branch: REL8_3_STABLE Release: REL8_3_6 [8d1d6019d] 2008-12-13 02:00:30 +0000
Branch: REL8_2_STABLE Release: REL8_2_12 [7ae3c0f67] 2008-12-13 02:00:53 +0000
Fix failure to ensure that a snapshot is available to datatype input functions
when they are invoked by the parser. We had been setting up a snapshot at
plan time but really it needs to be done earlier, before parse analysis.
Per report from Dmitry Koterov.
Also fix two related problems discovered while poking at this one:
exec_bind_message called datatype input functions without establishing a
snapshot, and SET CONSTRAINTS IMMEDIATE could call trigger functions without
establishing a snapshot.
Backpatch to 8.2. The underlying problem goes much further back, but it is
masked in 8.1 and before because we didn't attempt to invoke domain check
constraints within datatype input. It would only be exposed if a C-language
datatype input function used the snapshot; which evidently none do, or we'd
have heard complaints sooner. Since this code has changed a lot over time,
a back-patch is hardly risk-free, and so I'm disinclined to patch further
than absolutely necessary.
So if we take the complaint seriously, we'd better patch 8.2 as well.
regards, tom lane
On Wed, Dec 22, 2010 at 11:48 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Fujii Masao <masao.fujii@gmail.com> writes: >> The proposed patch looks very simple. I don't think that applying that >> patch will cause serious risk. > > Maybe so, maybe not, but *it won't get tested* to any meaningful degree > if it's applied. Umm.. I don't have good idea about this. Spend a lot time on the review? >> Unless the bug is fixed, the users who encountered the memory-leak >> cannot update their postgres to the latest version of 8.3. This would >> cause more serious situation. And, since psqlODBC frequently issues >> SAVEPONT and RELEASE SAVEPOINT, this memory-leak is not rare case, >> I think. > > The code's been like that for two years, and nobody's complained before > now, so it doesn't seem to me to be all that urgent. When I searched PostgreSQL mailing list archives, I found some related reports: http://archives.postgresql.org/pgsql-odbc/2009-05/msg00001.php http://archives.postgresql.org/pgsql-general/2009-04/msg00728.php >=A0Furthermore, the > people you are speaking of likely wouldn't upgrade anyway if they > haven't done so before now... Maybe. But, at least around me, software update is not performed so frequently since it requires the re-test of all related components. Many users (around me) decides whether to do the update in consideration of the budget and the influence of the bugs in the version they currently use. Regards, --=20 Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
On Wed, Dec 22, 2010 at 3:24 PM, Fujii Masao <masao.fujii@gmail.com> wrote: > On Wed, Dec 22, 2010 at 11:48 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Fujii Masao <masao.fujii@gmail.com> writes: >>> The proposed patch looks very simple. I don't think that applying that >>> patch will cause serious risk. >> >> Maybe so, maybe not, but *it won't get tested* to any meaningful degree >> if it's applied. > > Umm.. I don't have good idea about this. Spend a lot time on the review? The proposed patch changes only CopySnapshot in exec_simple_query. But exec_bind_message seems to have the same problem. It calls CopySnapshot on PortalHeapMemory, but calls FreeSnapshot on MessageContext. Am I missing something? Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
On Tue, Dec 21, 2010 at 11:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > So if we take the complaint seriously, we'd better patch 8.2 as well. I'm sort of inclined to think we should take the complaint seriously. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company