Properly initialize negative/empty cache entries in relfilenodemap

Started by Andres Freundover 12 years ago5 messages
#1Andres Freund
andres@2ndquadrant.com
1 attachment(s)

Hi,

Thanks to the valgrind instrumentation I found a small oversight in the
relfilenodemap patch which is fixed in the attached patch.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Attachments:

0001-relfilenodemap-Initialize-variable-properly-for-nega.patchtext/x-patch; charset=us-asciiDownload
>From bdab29233a0f47076b79015b6190d61dfed58d77 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Fri, 23 Aug 2013 15:32:56 +0200
Subject: [PATCH] relfilenodemap: Initialize variable properly for negative
 cache entries

---
 src/backend/utils/cache/relfilenodemap.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/backend/utils/cache/relfilenodemap.c b/src/backend/utils/cache/relfilenodemap.c
index 2a8f837..f3f9a09 100644
--- a/src/backend/utils/cache/relfilenodemap.c
+++ b/src/backend/utils/cache/relfilenodemap.c
@@ -180,6 +180,9 @@ RelidByRelfilenode(Oid reltablespace, Oid relfilenode)
 	if (found)
 		return entry->relid;
 
+	/* initialize empty/negative cache entry before doing the actual lookup */
+	entry->relid = InvalidOid;
+
 	/* ok, no previous cache entry, do it the hard way */
 
 	/* check shared tables */
-- 
1.8.3.251.g1462b67

#2Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Andres Freund (#1)
Re: Properly initialize negative/empty cache entries in relfilenodemap

Andres Freund wrote:

Hi,

Thanks to the valgrind instrumentation I found a small oversight in the
relfilenodemap patch which is fixed in the attached patch.

Pushed, thanks.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#3MauMau
maumau307@gmail.com
In reply to: Alvaro Herrera (#2)
Re: Properly initialize negative/empty cache entries in relfilenodemap

Andres Freund wrote:

Hi,

Thanks to the valgrind instrumentation I found a small oversight in the
relfilenodemap patch which is fixed in the attached patch.

Great! Could anybody find the root cause for the following memory leak
problem, and if possible, fix this?

/messages/by-id/214653D8DF574BFEAA6ED53E545E99E4@maumau

Heiki helped to solve this and found that pg_statistic entries are left in
CacheMemoryContext, but we have no idea where and how they are created and
left. This seems difficult to me.

Regards
MauMau

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#4Andres Freund
andres@2ndquadrant.com
In reply to: MauMau (#3)
Re: Properly initialize negative/empty cache entries in relfilenodemap

On 2013-08-29 21:35:13 +0900, MauMau wrote:

Andres Freund wrote:

Hi,

Thanks to the valgrind instrumentation I found a small oversight in the
relfilenodemap patch which is fixed in the attached patch.

Great! Could anybody find the root cause for the following memory leak
problem, and if possible, fix this?

/messages/by-id/214653D8DF574BFEAA6ED53E545E99E4@maumau

Heiki helped to solve this and found that pg_statistic entries are left in
CacheMemoryContext, but we have no idea where and how they are created and
left. This seems difficult to me.

That doesn't have anything to do with this patch/feature though, has it?
For one, the feature has only been integrated into HEAD after you've
reported the issue, for another, it's not even active unless you use it
the first time.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#5didier
did447@gmail.com
In reply to: MauMau (#3)
Re: Properly initialize negative/empty cache entries in relfilenodemap

Hi,

On Thu, Aug 29, 2013 at 2:35 PM, MauMau <maumau307@gmail.com> wrote:

Great! Could anybody find the root cause for the following memory leak
problem, and if possible, fix this?

http://www.postgresql.org/**message-id/**214653D8DF574BFEAA6ED53E545E99**
E4@maumau</messages/by-id/214653D8DF574BFEAA6ED53E545E99E4@maumau&gt;

Heiki helped to solve this and found that pg_statistic entries are left in
CacheMemoryContext, but we have no idea where and how they are created and
left. This seems difficult to me.

VALGRIND won't help you for this one

You hit 2 issues
- user can create negative cache entries in pg_statistic with SELECT but
they are unbound (at first there was a LRU aging but it was removed in 2006)

- if there's no row in pg_statistic for a relation/column then
RemoveStatistics, called by DROP ..., doesn't invalidate the cache (which
should remove these negative entries).