Re: [HACKERS] Protect syscache from bloating with negative cache entries

[Kyotaro HORIGUCHI]

This is a rebased version of the patch.

At Fri, 17 Mar 2017 14:23:13 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
 wrote in 
<20170317.142313.232290068.horiguchi.kyot...@lab.ntt.co.jp>
> At Tue, 7 Mar 2017 19:23:14 -0800, David Steele  wrote 
> in <3b7b7f90-db46-8c37-c4f7-443330c3a...@pgmasters.net>
> > On 3/3/17 4:54 PM, David Steele wrote:
> > 
> > > On 2/1/17 1:25 AM, Kyotaro HORIGUCHI wrote:
> > >> Hello, thank you for moving this to the next CF.
> > >>
> > >> At Wed, 1 Feb 2017 13:09:51 +0900, Michael Paquier
> > >>  wrote in
> > >> 
> > >>> On Tue, Jan 24, 2017 at 4:58 PM, Kyotaro HORIGUCHI
> > >>>  wrote:
> >  Six new syscaches in 665d1fa was conflicted and 3-way merge
> >  worked correctly. The new syscaches don't seem to be targets of
> >  this patch.
> > >>> To be honest, I am not completely sure what to think about this patch.
> > >>> Moved to next CF as there is a new version, and no new reviews to make
> > >>> the discussion perhaps move on.
> > >> I'm thinking the following is the status of this topic.
> > >>
> > >> - The patch stll is not getting conflicted.
> > >>
> > >> - This is not a hollistic measure for memory leak but surely
> > >>saves some existing cases.
> > >>
> > >> - Shared catcache is another discussion (and won't really
> > >>proposed in a short time due to the issue on locking.)
> > >>
> > >> - As I mentioned, a patch that caps the number of negative
> > >>entries is avaiable (in first-created - first-delete manner)
> > >>but it is having a loose end of how to determine the
> > >>limitation.
> > > While preventing bloat in the syscache is a worthwhile goal, it
> > > appears
> > > there are a number of loose ends here and a new patch has not been
> > > provided.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
>From 9f2c81dbc9bc344cafd6995dfc5969d55a8457d9 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Mon, 28 Aug 2017 11:36:21 +0900
Subject: [PATCH 1/2] Cleanup negative cache of pg_statistic when dropping a
 relation.

Accessing columns that don't have statistics leaves negative entries
in catcache for pg_statstic, but there's no chance to remove
them. Especially when repeatedly creating then dropping temporary
tables bloats catcache so much that memory pressure becomes
significant. This patch removes negative entries in STATRELATTINH,
ATTNAME and ATTNUM when corresponding relation is dropped.
---
 src/backend/utils/cache/catcache.c |  58 ++-
 src/backend/utils/cache/syscache.c | 302 +++--
 src/include/utils/catcache.h   |   3 +
 src/include/utils/syscache.h   |   2 +
 4 files changed, 282 insertions(+), 83 deletions(-)

diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c
index 95a0742..bd303f3 100644
--- a/src/backend/utils/cache/catcache.c
+++ b/src/backend/utils/cache/catcache.c
@@ -423,10 +423,11 @@ CatCachePrintStats(int code, Datum arg)
 
 		if (cache->cc_ntup == 0 && cache->cc_searches == 0)
 			continue;			/* don't print unused caches */
-		elog(DEBUG2, "catcache %s/%u: %d tup, %ld srch, %ld+%ld=%ld hits, %ld+%ld=%ld loads, %ld invals, %ld lsrch, %ld lhits",
+		elog(DEBUG2, "catcache %s/%u: %d tup, %d negtup, %ld srch, %ld+%ld=%ld hits, %ld+%ld=%ld loads, %ld invals, %ld lsrch, %ld lhits",
 			 cache->cc_relname,
 			 cache->cc_indexoid,
 			 cache->cc_ntup,
+			 cache->cc_nnegtup,
 			 cache->cc_searches,
 			 cache->cc_hits,
 			 cache->cc_neg_hits,
@@ -495,8 +496,11 @@ CatCacheRemoveCTup(CatCache *cache, CatCTup *ct)
 	 * point into tuple, allocated together with the CatCTup.
 	 */
 	if (ct->negative)
+	{
 		CatCacheFreeKeys(cache->cc_tupdesc, cache->cc_nkeys,
 		 cache->cc_keyno, ct->keys);
+		--cache->cc_nnegtup;
+	}
 
 	pfree(ct);
 
@@ -697,6 +701,51 @@ ResetCatalogCache(CatCache *cache)
 }
 
 /*
+ *		CleanupCatCacheNegEntries
+ *
+ *	Remove negative cache tuples matching a partial key.
+ *
+ */
+void
+CleanupCatCacheNegEntries(CatCache *cache, ScanKeyData *skey)
+{
+	int i;
+
+	/* If this cache has no negative entries, nothing to do */
+	if (cache->cc_nnegtup == 0)
+		return;
+
+	/* searching with a partial key means scanning the whole cache */
+	for (i = 0; i < cache->cc_nbuckets; i++)
+	{
+		dlist_head *bucket = >cc_bucket[i];
+		dlist_mutable_iter iter;
+
+		dlist_foreach_modify(iter, bucket)
+		{
+			const CCFastEqualFN *cc_fastequal = cache->cc_fastequal;
+			CatCTup*ct = dlist_container(CatCTup, cache_elem, iter.cur);
+			int			oid_attnum = skey->sk_attno - 1;
+
+			if (!ct->negative)
+continue;
+
+			/* Compare the OIDs */
+			if (!(cc_fastequal[oid_attnum])(ct->keys[oid_attnum],
+			skey[0].sk_argument))
+continue;
+
+			/*
+			 * the negative cache entries can no longer be 

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[Kyotaro HORIGUCHI]

Thank you for the comment.

At Mon, 28 Aug 2017 21:31:58 -0400, Robert Haas  wrote 
in 
> On Mon, Aug 28, 2017 at 5:24 AM, Kyotaro HORIGUCHI
>  wrote:
> > This patch have had interferences from several commits after the
> > last submission. I amended this patch to follow them (up to
> > f97c55c), removed an unnecessary branch and edited some comments.
> 
> I think the core problem for this patch is that there's no consensus
> on what approach to take.  Until that somehow gets sorted out, I think
> this isn't going to make any progress.  Unfortunately, I don't have a
> clear idea what sort of solution everybody could tolerate.
> 
> I still think that some kind of slow-expire behavior -- like a clock
> hand that hits each backend every 10 minutes and expires entries not
> used since the last hit -- is actually pretty sensible.  It ensures
> that idle or long-running backends don't accumulate infinite bloat
> while still allowing the cache to grow large enough for good
> performance when all entries are being regularly used.  But Tom
> doesn't like it.  Other approaches were also discussed; none of them
> seem like an obvious slam-dunk.

I suppose that it slows intermittent lookup of non-existent
objects. I have tried a slight different thing. Removing entries
by 'age', preserving specified number (or ratio to live entries)
of younger negative entries. The problem of that approach was I
didn't find how to determine the number of entries to preserve,
or I didn't want to offer additional knobs for them. Finally I
proposed the patch upthread since it doesn't need any assumption
on usage.

Though I can make another patch that does the same thing based on
LRU, the same how-many-to-preserve problem ought to be resolved
in order to avoid slowing the inermittent lookup.

> Turning to the patch itself, I don't know how we decide whether the
> patch is worth it.  Scanning the whole (potentially large) cache to
> remove negative entries has a cost, mostly in CPU cycles; keeping
> those negative entries around for a long time also has a cost, mostly
> in memory.  I don't know how to decide whether these patches will help
> more people than it hurts, or the other way around -- and it's not
> clear that anyone else has a good idea about that either.

Scanning a hash on invalidation of several catalogs (hopefully
slightly) slows certain percentage of inavlidations on maybe most
of workloads. Holding no-longer-lookedup entries surely kills a
backend under certain workloads sooner or later.  This doesn't
save the pg_proc cases, but saves pg_statistic and pg_class
cases. I'm not sure what other catalogs can bloat.

I could reduce the complexity of this. Inval mechanism conveys
only a hash value so this scans the whole of a cache for the
target OIDs (with possible spurious targets). This will be
resolved by letting inval mechanism convey an OID. (but this may
need additional members in an inval entry.)

Still, the full scan perfomed in CleanupCatCacheNegEntries
doesn't seem easily avoidable. Separating the hash by OID of key
or provide special dlist that points tuples in buckets will
introduce another complexity.


> Typos: funciton, paritial.

Thanks. ispell told me of additional typos corresnpond, belive
and undistinguisable.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



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

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[Kyotaro HORIGUCHI]

Thank you for reviewing this.

At Sat, 2 Sep 2017 12:12:47 +1200, Thomas Munro  
wrote in 

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[Thomas Munro]

On Mon, Aug 28, 2017 at 9:24 PM, Kyotaro HORIGUCHI
 wrote:
> This patch have had interferences from several commits after the
> last submission. I amended this patch to follow them (up to
> f97c55c), removed an unnecessary branch and edited some comments.

Hi Kyotaro-san,

This applies but several regression tests fail for me.  Here is a
sample backtrace:

frame #3: 0x00010f0614c0
postgres`ExceptionalCondition(conditionName="!(attnum < 0 ? attnum ==
(-2) : cache->cc_tupdesc->attrs[attnum].atttypid == 26)",
errorType="FailedAssertion", fileName="catcache.c", lineNumber=1384) +
128 at assert.c:54
frame #4: 0x00010f03b5fd
postgres`CollectOIDsForHashValue(cache=0x7fe273821268,
hashValue=994410284, attnum=0) + 253 at catcache.c:1383
frame #5: 0x00010f055e8e
postgres`SysCacheSysCacheInvalCallback(arg=140610577303984, cacheid=0,
hashValue=994410284) + 94 at syscache.c:1692
frame #6: 0x00010f03fbbb
postgres`CallSyscacheCallbacks(cacheid=0, hashvalue=994410284) + 219
at inval.c:1468
frame #7: 0x00010f03f878
postgres`LocalExecuteInvalidationMessage(msg=0x7fff51213ff8) + 88
at inval.c:566
frame #8: 0x00010ee7a3f2
postgres`ReceiveSharedInvalidMessages(invalFunction=(postgres`LocalExecuteInvalidationMessage
at inval.c:555), resetFunction=(postgres`InvalidateSystemCaches at
inval.c:647)) + 354 at sinval.c:121
frame #9: 0x00010f03fcb7 postgres`AcceptInvalidationMessages +
23 at inval.c:686
frame #10: 0x00010eade609 postgres`AtStart_Cache + 9 at xact.c:987
frame #11: 0x00010ead8c2f postgres`StartTransaction + 655 at xact.c:1921
frame #12: 0x00010ead8896 postgres`StartTransactionCommand +
70 at xact.c:2691
frame #13: 0x00010eea9746 postgres`start_xact_command + 22 at
postgres.c:2438
frame #14: 0x00010eea722e
postgres`exec_simple_query(query_string="RESET SESSION
AUTHORIZATION;") + 126 at postgres.c:913
frame #15: 0x00010eea68d7 postgres`PostgresMain(argc=1,
argv=0x7fe2738036a8, dbname="regression", username="munro") + 2375
at postgres.c:4090
frame #16: 0x00010eded40e
postgres`BackendRun(port=0x7fe2716001a0) + 654 at
postmaster.c:4357
frame #17: 0x00010edec793
postgres`BackendStartup(port=0x7fe2716001a0) + 483 at
postmaster.c:4029
frame #18: 0x00010edeb785 postgres`ServerLoop + 597 at postmaster.c:1753
frame #19: 0x00010ede8f71 postgres`PostmasterMain(argc=8,
argv=0x7fe271403860) + 5553 at postmaster.c:1361
frame #20: 0x00010ed0ccd9 postgres`main(argc=8,
argv=0x7fe271403860) + 761 at main.c:228
frame #21: 0x7fff8333a5ad libdyld.dylib`start + 1

-- 
Thomas Munro
http://www.enterprisedb.com


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

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[Robert Haas]

On Mon, Aug 28, 2017 at 5:24 AM, Kyotaro HORIGUCHI
 wrote:
> This patch have had interferences from several commits after the
> last submission. I amended this patch to follow them (up to
> f97c55c), removed an unnecessary branch and edited some comments.

I think the core problem for this patch is that there's no consensus
on what approach to take.  Until that somehow gets sorted out, I think
this isn't going to make any progress.  Unfortunately, I don't have a
clear idea what sort of solution everybody could tolerate.

I still think that some kind of slow-expire behavior -- like a clock
hand that hits each backend every 10 minutes and expires entries not
used since the last hit -- is actually pretty sensible.  It ensures
that idle or long-running backends don't accumulate infinite bloat
while still allowing the cache to grow large enough for good
performance when all entries are being regularly used.  But Tom
doesn't like it.  Other approaches were also discussed; none of them
seem like an obvious slam-dunk.

Turning to the patch itself, I don't know how we decide whether the
patch is worth it.  Scanning the whole (potentially large) cache to
remove negative entries has a cost, mostly in CPU cycles; keeping
those negative entries around for a long time also has a cost, mostly
in memory.  I don't know how to decide whether these patches will help
more people than it hurts, or the other way around -- and it's not
clear that anyone else has a good idea about that either.

Typos: funciton, paritial.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[Kyotaro HORIGUCHI]

Thank you for your attention.

At Mon, 14 Aug 2017 17:33:48 -0400, Peter Eisentraut 
 wrote in 
<09fa011f-4536-b05d-0625-11f3625d8...@2ndquadrant.com>
> On 1/24/17 02:58, Kyotaro HORIGUCHI wrote:
> >> BTW, if you set a slightly larger
> >> context size on the patch you might be able to avoid rebases; right
> >> now the patch doesn't include enough context to uniquely identify the
> >> chunks against cacheinfo[].
> > git format-patch -U5 fuses all hunks on cacheinfo[] together. I'm
> > not sure that such a hunk can avoid rebases. Is this what you
> > suggested? -U4 added an identifiable forward context line for
> > some elements so the attached patch is made with four context
> > lines.
> 
> This patch needs another rebase for the upcoming commit fest.

This patch have had interferences from several commits after the
last submission. I amended this patch to follow them (up to
f97c55c), removed an unnecessary branch and edited some comments.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
>From 4887f7d178b41a1a4729931a12bd396b9a8e8ee0 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Mon, 28 Aug 2017 11:36:21 +0900
Subject: [PATCH 1/2] Cleanup negative cache of pg_statistic when dropping a
 relation.

Accessing columns that don't have statistics leaves negative entries
in catcache for pg_statstic, but there's no chance to remove
them. Especially when repeatedly creating then dropping temporary
tables bloats catcache so much that memory pressure becomes
significant. This patch removes negative entries in STATRELATTINH,
ATTNAME and ATTNUM when corresponding relation is dropped.
---
 src/backend/utils/cache/catcache.c |  57 ++-
 src/backend/utils/cache/syscache.c | 299 +++--
 src/include/utils/catcache.h   |   3 +
 src/include/utils/syscache.h   |   2 +
 4 files changed, 279 insertions(+), 82 deletions(-)

diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c
index e092801..e50c997 100644
--- a/src/backend/utils/cache/catcache.c
+++ b/src/backend/utils/cache/catcache.c
@@ -304,10 +304,11 @@ CatCachePrintStats(int code, Datum arg)
 
 		if (cache->cc_ntup == 0 && cache->cc_searches == 0)
 			continue;			/* don't print unused caches */
-		elog(DEBUG2, "catcache %s/%u: %d tup, %ld srch, %ld+%ld=%ld hits, %ld+%ld=%ld loads, %ld invals, %ld lsrch, %ld lhits",
+		elog(DEBUG2, "catcache %s/%u: %d tup, %d negtup, %ld srch, %ld+%ld=%ld hits, %ld+%ld=%ld loads, %ld invals, %ld lsrch, %ld lhits",
 			 cache->cc_relname,
 			 cache->cc_indexoid,
 			 cache->cc_ntup,
+			 cache->cc_nnegtup,
 			 cache->cc_searches,
 			 cache->cc_hits,
 			 cache->cc_neg_hits,
@@ -374,6 +375,10 @@ CatCacheRemoveCTup(CatCache *cache, CatCTup *ct)
 	/* free associated tuple data */
 	if (ct->tuple.t_data != NULL)
 		pfree(ct->tuple.t_data);
+
+	if (ct->negative)
+		--cache->cc_nnegtup;
+
 	pfree(ct);
 
 	--cache->cc_ntup;
@@ -572,6 +577,49 @@ ResetCatalogCache(CatCache *cache)
 }
 
 /*
+ *		CleanupCatCacheNegEntries
+ *
+ *	Remove negative cache tuples matching a partial key.
+ *
+ */
+void
+CleanupCatCacheNegEntries(CatCache *cache, ScanKeyData *skey)
+{
+	int i;
+
+	/* If this cache has no negative entries, nothing to do */
+	if (cache->cc_nnegtup == 0)
+		return;
+
+	/* searching with a paritial key means scanning the whole cache */
+	for (i = 0; i < cache->cc_nbuckets; i++)
+	{
+		dlist_head *bucket = >cc_bucket[i];
+		dlist_mutable_iter iter;
+
+		dlist_foreach_modify(iter, bucket)
+		{
+			CatCTup*ct = dlist_container(CatCTup, cache_elem, iter.cur);
+			bool		res;
+
+			if (!ct->negative)
+continue;
+
+			HeapKeyTest(>tuple, cache->cc_tupdesc, 1, skey, res);
+			if (!res)
+continue;
+
+			/*
+			 * the negative cache entries can no longer be referenced, so we
+			 * can remove it unconditionally
+			 */
+			CatCacheRemoveCTup(cache, ct);
+		}
+	}
+}
+
+
+/*
  *		ResetCatalogCaches
  *
  * Reset all caches when a shared cache inval event forces it
@@ -718,6 +766,7 @@ InitCatCache(int id,
 	cp->cc_relisshared = false; /* temporary */
 	cp->cc_tupdesc = (TupleDesc) NULL;
 	cp->cc_ntup = 0;
+	cp->cc_nnegtup = 0;
 	cp->cc_nbuckets = nbuckets;
 	cp->cc_nkeys = nkeys;
 	for (i = 0; i < nkeys; ++i)
@@ -1215,8 +1264,8 @@ SearchCatCache(CatCache *cache,
 
 		CACHE4_elog(DEBUG2, "SearchCatCache(%s): Contains %d/%d tuples",
 	cache->cc_relname, cache->cc_ntup, CacheHdr->ch_ntup);
-		CACHE3_elog(DEBUG2, "SearchCatCache(%s): put neg entry in bucket %d",
-	cache->cc_relname, hashIndex);
+		CACHE4_elog(DEBUG2, "SearchCatCache(%s): put neg entry in bucket %d, total %d",
+	cache->cc_relname, hashIndex, cache->cc_nnegtup);
 
 		/*
 		 * We are not returning the negative entry to the caller, so leave its
@@ -1667,6 +1716,8 @@ CatalogCacheCreateEntry(CatCache *cache, HeapTuple ntp,
 
 	cache->cc_ntup++;
 	CacheHdr->ch_ntup++;
+	if (negative)
+		cache->cc_nnegtup++;
 
 	/*

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[Peter Eisentraut]

On 1/24/17 02:58, Kyotaro HORIGUCHI wrote:
>> BTW, if you set a slightly larger
>> context size on the patch you might be able to avoid rebases; right
>> now the patch doesn't include enough context to uniquely identify the
>> chunks against cacheinfo[].
> git format-patch -U5 fuses all hunks on cacheinfo[] together. I'm
> not sure that such a hunk can avoid rebases. Is this what you
> suggested? -U4 added an identifiable forward context line for
> some elements so the attached patch is made with four context
> lines.

This patch needs another rebase for the upcoming commit fest.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[Kyotaro HORIGUCHI]

At Tue, 7 Mar 2017 19:23:14 -0800, David Steele  wrote in 
<3b7b7f90-db46-8c37-c4f7-443330c3a...@pgmasters.net>
> On 3/3/17 4:54 PM, David Steele wrote:
> 
> > On 2/1/17 1:25 AM, Kyotaro HORIGUCHI wrote:
> >> Hello, thank you for moving this to the next CF.
> >>
> >> At Wed, 1 Feb 2017 13:09:51 +0900, Michael Paquier
> >>  wrote in
> >> 
> >>> On Tue, Jan 24, 2017 at 4:58 PM, Kyotaro HORIGUCHI
> >>>  wrote:
>  Six new syscaches in 665d1fa was conflicted and 3-way merge
>  worked correctly. The new syscaches don't seem to be targets of
>  this patch.
> >>> To be honest, I am not completely sure what to think about this patch.
> >>> Moved to next CF as there is a new version, and no new reviews to make
> >>> the discussion perhaps move on.
> >> I'm thinking the following is the status of this topic.
> >>
> >> - The patch stll is not getting conflicted.
> >>
> >> - This is not a hollistic measure for memory leak but surely
> >>saves some existing cases.
> >>
> >> - Shared catcache is another discussion (and won't really
> >>proposed in a short time due to the issue on locking.)
> >>
> >> - As I mentioned, a patch that caps the number of negative
> >>entries is avaiable (in first-created - first-delete manner)
> >>but it is having a loose end of how to determine the
> >>limitation.
> > While preventing bloat in the syscache is a worthwhile goal, it
> > appears
> > there are a number of loose ends here and a new patch has not been
> > provided.
> >
> > It's a pretty major change so I recommend moving this patch to the
> > 2017-07 CF.
> 
> Not hearing any opinions pro or con, I'm moving this patch to the
> 2017-07 CF.

Ah. Yes, I agree on this. Thanks.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



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

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[David Steele]

On 3/3/17 4:54 PM, David Steele wrote:


On 2/1/17 1:25 AM, Kyotaro HORIGUCHI wrote:

Hello, thank you for moving this to the next CF.

At Wed, 1 Feb 2017 13:09:51 +0900, Michael Paquier  wrote 
in 

On Tue, Jan 24, 2017 at 4:58 PM, Kyotaro HORIGUCHI
 wrote:

Six new syscaches in 665d1fa was conflicted and 3-way merge
worked correctly. The new syscaches don't seem to be targets of
this patch.

To be honest, I am not completely sure what to think about this patch.
Moved to next CF as there is a new version, and no new reviews to make
the discussion perhaps move on.

I'm thinking the following is the status of this topic.

- The patch stll is not getting conflicted.

- This is not a hollistic measure for memory leak but surely
   saves some existing cases.

- Shared catcache is another discussion (and won't really
   proposed in a short time due to the issue on locking.)

- As I mentioned, a patch that caps the number of negative
   entries is avaiable (in first-created - first-delete manner)
   but it is having a loose end of how to determine the
   limitation.

While preventing bloat in the syscache is a worthwhile goal, it appears
there are a number of loose ends here and a new patch has not been provided.

It's a pretty major change so I recommend moving this patch to the
2017-07 CF.


Not hearing any opinions pro or con, I'm moving this patch to the 
2017-07 CF.


--
-David
da...@pgmasters.net



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

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[David Steele]

On 2/1/17 1:25 AM, Kyotaro HORIGUCHI wrote:
> Hello, thank you for moving this to the next CF.
> 
> At Wed, 1 Feb 2017 13:09:51 +0900, Michael Paquier 
>  wrote in 
> 
>> On Tue, Jan 24, 2017 at 4:58 PM, Kyotaro HORIGUCHI
>>  wrote:
>>> Six new syscaches in 665d1fa was conflicted and 3-way merge
>>> worked correctly. The new syscaches don't seem to be targets of
>>> this patch.
>>
>> To be honest, I am not completely sure what to think about this patch.
>> Moved to next CF as there is a new version, and no new reviews to make
>> the discussion perhaps move on.
> 
> I'm thinking the following is the status of this topic.
> 
> - The patch stll is not getting conflicted.
> 
> - This is not a hollistic measure for memory leak but surely
>   saves some existing cases.
> 
> - Shared catcache is another discussion (and won't really
>   proposed in a short time due to the issue on locking.)
> 
> - As I mentioned, a patch that caps the number of negative
>   entries is avaiable (in first-created - first-delete manner)
>   but it is having a loose end of how to determine the
>   limitation.

While preventing bloat in the syscache is a worthwhile goal, it appears
there are a number of loose ends here and a new patch has not been provided.

It's a pretty major change so I recommend moving this patch to the
2017-07 CF.

-- 
-David
da...@pgmasters.net


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

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[Kyotaro HORIGUCHI]

Hello, thank you for moving this to the next CF.

At Wed, 1 Feb 2017 13:09:51 +0900, Michael Paquier  
wrote in 
> On Tue, Jan 24, 2017 at 4:58 PM, Kyotaro HORIGUCHI
>  wrote:
> > Six new syscaches in 665d1fa was conflicted and 3-way merge
> > worked correctly. The new syscaches don't seem to be targets of
> > this patch.
> 
> To be honest, I am not completely sure what to think about this patch.
> Moved to next CF as there is a new version, and no new reviews to make
> the discussion perhaps move on.

I'm thinking the following is the status of this topic.

- The patch stll is not getting conflicted.

- This is not a hollistic measure for memory leak but surely
  saves some existing cases.

- Shared catcache is another discussion (and won't really
  proposed in a short time due to the issue on locking.)

- As I mentioned, a patch that caps the number of negative
  entries is avaiable (in first-created - first-delete manner)
  but it is having a loose end of how to determine the
  limitation.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[Michael Paquier]

On Tue, Jan 24, 2017 at 4:58 PM, Kyotaro HORIGUCHI
 wrote:
> Six new syscaches in 665d1fa was conflicted and 3-way merge
> worked correctly. The new syscaches don't seem to be targets of
> this patch.

To be honest, I am not completely sure what to think about this patch.
Moved to next CF as there is a new version, and no new reviews to make
the discussion perhaps move on.
-- 
Michael


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

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[Kyotaro HORIGUCHI]

Hello,

I have tried to cap the number of negative entries for myself (by
removing negative entries in least recentrly created first order)
but the ceils cannot be reasonably determined both absolutely or
relatively to positive entries. Apparently it differs widely
among caches and applications.

At Mon, 23 Jan 2017 08:16:49 -0600, Jim Nasby  wrote 
in <6519b7ad-0aa6-c9f4-8869-20691107f...@bluetreble.com>
> On 1/22/17 5:03 PM, Tom Lane wrote:
> >> Ok, after reading the code I see I only partly understood what you
> >> were
> >> saying. In any case, it might still be useful to do some testing with
> >> CATCACHE_STATS defined to see if there's caches that don't accumulate
> >> a
> >> lot of negative entries.
> > There definitely are, according to my testing, but by the same token
> > it's not clear that a shutoff check would save anything.
> 
> Currently they wouldn't, but there's concerns about the performance of
> some of the other ideas in this thread. Getting rid of negative
> entries that don't really help could reduce some of those concerns. Or
> perhaps the original complaint about STATRELATTINH could be solved by
> just disabling negative entries on that cache.

As for STATRELATTINH, planning involving small temporary tables
 that frequently accessed willget benefit from negative entries,
 but it might ignorably small. ATTNAME, ATTNUM and RENAMENSP also
 might not get so much from negative entries. If these are true,
 the whole stuff this patch adds can be replaced with just a
 boolean in cachedesc that inhibits negatvie entries. Anyway this
 patch don't save the case of the cache bloat relaed to function
 reference. I'm not sure how that could be reproduced, though.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[Kyotaro HORIGUCHI]

Hello, thank you for lookin this.

At Mon, 23 Jan 2017 16:54:36 -0600, Jim Nasby  wrote 
in <21803f50-a823-c444-ee2b-9a153114f...@bluetreble.com>
> On 1/21/17 6:42 PM, Jim Nasby wrote:
> > On 12/26/16 2:31 AM, Kyotaro HORIGUCHI wrote:
> >> The points of discussion are the following, I think.
> >>
> >> 1. The first patch seems working well. It costs the time to scan
> >>the whole of a catcache that have negative entries for other
> >>reloids. However, such negative entries are created by rather
> >>unusual usages. Accesing to undefined columns, and accessing
> >>columns on which no statistics have created. The
> >>whole-catcache scan occurs on ATTNAME, ATTNUM and
> >>STATRELATTINH for every invalidation of a relcache entry.
> >
> > I took a look at this. It looks sane, though I've got a few minor
> > comment tweaks:
> >
> > + *Remove negative cache tuples maching a partial key.
> > s/maching/matching/
> >
> > +/* searching with a paritial key needs scanning the whole cache */
> >
> > s/needs/means/
> >
> > + * a negative cache entry cannot be referenced so we can remove
> >
> > s/referenced/referenced,/
> >
> > I was wondering if there's a way to test the performance impact of
> > deleting negative entries.

Thanks for the pointing out. These are addressed.

> I did a make installcheck run with CATCACHE_STATS to see how often we
> get negative entries in the 3 caches affected by this patch. The
> caches on pg_attribute get almost no negative entries. pg_statistic
> gets a good amount of negative entries, presumably because we start
> off with no entries in there. On a stable system that presumably won't
> be an issue, but if temporary tables are in use and being analyzed I'd
> think there could be a moderate amount of inval traffic on that
> cache. I'll leave it to a committer to decide if they thing that's an
> issue, but you might want to try and quantify how big a hit that is. I
> think it'd also be useful to know how much bloat you were seeing in
> the field.
> 
> The patch is currently conflicting against master though, due to some
> caches being added. Can you rebase?

Six new syscaches in 665d1fa was conflicted and 3-way merge
worked correctly. The new syscaches don't seem to be targets of
this patch.

> BTW, if you set a slightly larger
> context size on the patch you might be able to avoid rebases; right
> now the patch doesn't include enough context to uniquely identify the
> chunks against cacheinfo[].

git format-patch -U5 fuses all hunks on cacheinfo[] together. I'm
not sure that such a hunk can avoid rebases. Is this what you
suggested? -U4 added an identifiable forward context line for
some elements so the attached patch is made with four context
lines.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
>From 3e9932cff6a2db1d081821269f1bab8b39b0f4f0 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Thu, 15 Dec 2016 17:43:03 +0900
Subject: [PATCH 1/2] Cleanup negative cache of pg_statistic when dropping a
 relation.

Accessing columns that don't have statistics causes leaves negative
entries in catcache for pg_statstic, but there's no chance to remove
them. Especially when repeatedly creating then dropping temporary
tables bloats catcache so much that memory pressure can be
significant. This patch removes negative entries in STATRELATTINH,
ATTNAME and ATTNUM when corresponding relation is dropped.
---
 src/backend/utils/cache/catcache.c |  57 ++-
 src/backend/utils/cache/syscache.c | 295 +++--
 src/include/utils/catcache.h   |   3 +
 src/include/utils/syscache.h   |   2 +
 4 files changed, 277 insertions(+), 80 deletions(-)

diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c
index c27186f..6ff2c5e 100644
--- a/src/backend/utils/cache/catcache.c
+++ b/src/backend/utils/cache/catcache.c
@@ -303,12 +303,13 @@ CatCachePrintStats(int code, Datum arg)
 		CatCache   *cache = slist_container(CatCache, cc_next, iter.cur);
 
 		if (cache->cc_ntup == 0 && cache->cc_searches == 0)
 			continue;			/* don't print unused caches */
-		elog(DEBUG2, "catcache %s/%u: %d tup, %ld srch, %ld+%ld=%ld hits, %ld+%ld=%ld loads, %ld invals, %ld lsrch, %ld lhits",
+		elog(DEBUG2, "catcache %s/%u: %d tup, %d negtup, %ld srch, %ld+%ld=%ld hits, %ld+%ld=%ld loads, %ld invals, %ld lsrch, %ld lhits",
 			 cache->cc_relname,
 			 cache->cc_indexoid,
 			 cache->cc_ntup,
+			 cache->cc_nnegtup,
 			 cache->cc_searches,
 			 cache->cc_hits,
 			 cache->cc_neg_hits,
 			 cache->cc_hits + cache->cc_neg_hits,
@@ -373,8 +374,12 @@ CatCacheRemoveCTup(CatCache *cache, CatCTup *ct)
 
 	/* free associated tuple data */
 	if (ct->tuple.t_data != NULL)
 		pfree(ct->tuple.t_data);
+
+	if (ct->negative)
+		--cache->cc_nnegtup;
+
 	pfree(ct);
 
 	--cache->cc_ntup;
 	--CacheHdr->ch_ntup;
@@ -636,8 +641,51 @@ ResetCatalogCache(CatCache *cache)
 	}
 }
 
 /*
+ *		

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[Jim Nasby]

On 1/21/17 6:42 PM, Jim Nasby wrote:

On 12/26/16 2:31 AM, Kyotaro HORIGUCHI wrote:

The points of discussion are the following, I think.

1. The first patch seems working well. It costs the time to scan
   the whole of a catcache that have negative entries for other
   reloids. However, such negative entries are created by rather
   unusual usages. Accesing to undefined columns, and accessing
   columns on which no statistics have created. The
   whole-catcache scan occurs on ATTNAME, ATTNUM and
   STATRELATTINH for every invalidation of a relcache entry.


I took a look at this. It looks sane, though I've got a few minor
comment tweaks:

+ *Remove negative cache tuples maching a partial key.
s/maching/matching/

+/* searching with a paritial key needs scanning the whole cache */

s/needs/means/

+ * a negative cache entry cannot be referenced so we can remove

s/referenced/referenced,/

I was wondering if there's a way to test the performance impact of
deleting negative entries.


I did a make installcheck run with CATCACHE_STATS to see how often we 
get negative entries in the 3 caches affected by this patch. The caches 
on pg_attribute get almost no negative entries. pg_statistic gets a good 
amount of negative entries, presumably because we start off with no 
entries in there. On a stable system that presumably won't be an issue, 
but if temporary tables are in use and being analyzed I'd think there 
could be a moderate amount of inval traffic on that cache. I'll leave it 
to a committer to decide if they thing that's an issue, but you might 
want to try and quantify how big a hit that is. I think it'd also be 
useful to know how much bloat you were seeing in the field.


The patch is currently conflicting against master though, due to some 
caches being added. Can you rebase? BTW, if you set a slightly larger 
context size on the patch you might be able to avoid rebases; right now 
the patch doesn't include enough context to uniquely identify the chunks 
against cacheinfo[].

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


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

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[Jim Nasby]

On 1/22/17 5:03 PM, Tom Lane wrote:

Ok, after reading the code I see I only partly understood what you were
saying. In any case, it might still be useful to do some testing with
CATCACHE_STATS defined to see if there's caches that don't accumulate a
lot of negative entries.

There definitely are, according to my testing, but by the same token
it's not clear that a shutoff check would save anything.


Currently they wouldn't, but there's concerns about the performance of 
some of the other ideas in this thread. Getting rid of negative entries 
that don't really help could reduce some of those concerns. Or perhaps 
the original complaint about STATRELATTINH could be solved by just 
disabling negative entries on that cache.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


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

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[Tom Lane]

Jim Nasby  writes:
>> Ahh, I hadn't considered that. So one idea would be to only track
>> negative entries on caches where we know they're actually useful. That
>> might make the performance hit of some of the other ideas more
>> tolerable. Presumably you're much less likely to pollute the namespace
>> cache than some of the others.

> Ok, after reading the code I see I only partly understood what you were 
> saying. In any case, it might still be useful to do some testing with 
> CATCACHE_STATS defined to see if there's caches that don't accumulate a 
> lot of negative entries.

There definitely are, according to my testing, but by the same token
it's not clear that a shutoff check would save anything.

regards, tom lane


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

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[Jim Nasby]

On 1/22/17 4:41 PM, Jim Nasby wrote:

On 1/21/17 8:54 PM, Tom Lane wrote:

Jim Nasby  writes:

The other (possibly naive) question I have is how useful negative
entries really are? Will Postgres regularly incur negative lookups, or
will these only happen due to user activity?

It varies depending on the particular syscache, but in at least some
of them, negative cache entries are critical for performance.
See for example RelnameGetRelid(), which basically does a RELNAMENSP
cache lookup for each schema down the search path until it finds a
match.


Ahh, I hadn't considered that. So one idea would be to only track
negative entries on caches where we know they're actually useful. That
might make the performance hit of some of the other ideas more
tolerable. Presumably you're much less likely to pollute the namespace
cache than some of the others.


Ok, after reading the code I see I only partly understood what you were 
saying. In any case, it might still be useful to do some testing with 
CATCACHE_STATS defined to see if there's caches that don't accumulate a 
lot of negative entries.


Attached is a patch that tries to document some of this.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)
diff --git a/src/include/utils/catcache.h b/src/include/utils/catcache.h
index 253c7b53ed..d718d82d80 100644
--- a/src/include/utils/catcache.h
+++ b/src/include/utils/catcache.h
@@ -101,7 +101,12 @@ typedef struct catctup
 *
 * A negative cache entry is an assertion that there is no tuple 
matching
 * a particular key.  This is just as useful as a normal entry so far as
-* avoiding catalog searches is concerned.  Management of positive and
+* avoiding catalog searches is concerned.  In particular, caching 
negative
+* entries is critical for performance of some caches. For example, 
current
+* code will produce a negative RELNAMENSP entry for every un-qualified
+* table looking with the default search_path that puts the pg_catalog
+* schema first. This effect can be obverved by defining CATCACHE_STATS 
and
+* observing the log at backend exit.  Management of positive and
 * negative entries is identical.
 */
int refcount;   /* number of active 
references */

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

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[Jim Nasby]

On 1/21/17 8:54 PM, Tom Lane wrote:

Jim Nasby  writes:

The other (possibly naive) question I have is how useful negative
entries really are? Will Postgres regularly incur negative lookups, or
will these only happen due to user activity?

It varies depending on the particular syscache, but in at least some
of them, negative cache entries are critical for performance.
See for example RelnameGetRelid(), which basically does a RELNAMENSP
cache lookup for each schema down the search path until it finds a
match.


Ahh, I hadn't considered that. So one idea would be to only track 
negative entries on caches where we know they're actually useful. That 
might make the performance hit of some of the other ideas more 
tolerable. Presumably you're much less likely to pollute the namespace 
cache than some of the others.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


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

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[Tom Lane]

Jim Nasby  writes:
> The other (possibly naive) question I have is how useful negative 
> entries really are? Will Postgres regularly incur negative lookups, or 
> will these only happen due to user activity?

It varies depending on the particular syscache, but in at least some
of them, negative cache entries are critical for performance.
See for example RelnameGetRelid(), which basically does a RELNAMENSP
cache lookup for each schema down the search path until it finds a
match.  For any user table name with the standard search_path, there's
a guaranteed failure in pg_catalog before you can hope to find a match.
If we don't have negative cache entries, then *every invocation of this
function has to go to disk* (or at least to shared buffers).

It's possible that we could revise all our lookup patterns to avoid this
sort of thing.  But I don't have much faith in that always being possible,
and exactly none that we won't introduce new lookup patterns that need it
in future.  I spent some time, for instance, wondering if RelnameGetRelid
could use a SearchSysCacheList lookup instead, doing the lookup on table
name only and then inspecting the whole list to see which entry is
frontmost according to the current search path.  But that has performance
failure modes of its own, for example if you have identical table names in
a boatload of different schemas.  We do it that way for some other cases
such as function lookups, but I think it's much less likely that people
have identical function names in N schemas than that they have identical
table names in N schemas.

If you want to poke into this for particular test scenarios, building with
CATCACHE_STATS defined will yield a bunch of numbers dumped to the
postmaster log at each backend exit.

regards, tom lane


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

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[Jim Nasby]

On 12/26/16 2:31 AM, Kyotaro HORIGUCHI wrote:

The points of discussion are the following, I think.

1. The first patch seems working well. It costs the time to scan
   the whole of a catcache that have negative entries for other
   reloids. However, such negative entries are created by rather
   unusual usages. Accesing to undefined columns, and accessing
   columns on which no statistics have created. The
   whole-catcache scan occurs on ATTNAME, ATTNUM and
   STATRELATTINH for every invalidation of a relcache entry.


I took a look at this. It looks sane, though I've got a few minor 
comment tweaks:


+ * Remove negative cache tuples maching a partial key.
s/maching/matching/

+/* searching with a paritial key needs scanning the whole cache */

s/needs/means/

+ * a negative cache entry cannot be referenced so we can remove

s/referenced/referenced,/

I was wondering if there's a way to test the performance impact of 
deleting negative entries.



2. The second patch also works, but flushing negative entries by
   hash values is inefficient. It scans the bucket corresponding
   to given hash value for OIDs, then flushing negative entries
   iterating over all the collected OIDs. So this costs more time
   than 1 and flushes involving entries that is not necessary to
   be removed. If this feature is valuable but such side effects
   are not acceptable, new invalidation category based on
   cacheid-oid pair would be needed.


I glanced at this and it looks sane. Didn't go any farther since this 
one's pretty up in the air. ISTM it'd be better to do some kind of aging 
instead of patch 2.


The other (possibly naive) question I have is how useful negative 
entries really are? Will Postgres regularly incur negative lookups, or 
will these only happen due to user activity? I can't think of a case 
where an app would need to depend on fast negative lookup (in other 
words, it should be considered a bug in the app). I can see where 
getting rid of them completely might be problematic, but maybe we can 
just keep a relatively small number of them around. I'm thinking a 
simple LRU list of X number of negative entries; when that fills you 
reuse the oldest one. You'd have to pay the LRU maintenance cost on 
every negative hit, but if those shouldn't be that common it shouldn't 
be bad.


That might well necessitate another GUC, but it seems a lot simpler than 
most of the other ideas.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


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

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[Michael Paquier]

On Sat, Jan 14, 2017 at 9:36 AM, Tomas Vondra
 wrote:
> On 01/14/2017 12:06 AM, Andres Freund wrote:
>> On 2017-01-13 17:58:41 -0500, Tom Lane wrote:
>>>
>>> But, again, the catcache isn't the only source of per-process bloat
>>> and I'm not even sure it's the main one.  A more holistic approach
>>> might be called for.
>>
>> It'd be helpful if we'd find a way to make it easy to get statistics
>> about the size of various caches in production systems. Right now
>> that's kinda hard, resulting in us having to make a lot of
>> guesses...
>
> What about a simple C extension, that could inspect those caches? Assuming
> it could be loaded into a single backend, that should be relatively
> acceptable way (compared to loading it to all backends using
> shared_preload_libraries).

This extension could do a small amount of work on a portion of the
syscache entries at each query loop, still I am wondering if that
would not be nicer to get that in-core and configurable, which is
basically the approach proposed by Horiguchi-san. At least it seems to
me that it has some merit, and if we could make that behavior
switchable, disabled by default, that would be a win for some class of
applications. What do others think?
-- 
Michael


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

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[Tomas Vondra]

On 01/14/2017 12:06 AM, Andres Freund wrote:

Hi,


On 2017-01-13 17:58:41 -0500, Tom Lane wrote:

But, again, the catcache isn't the only source of per-process bloat
and I'm not even sure it's the main one.  A more holistic approach
might be called for.


It'd be helpful if we'd find a way to make it easy to get statistics
about the size of various caches in production systems. Right now
that's kinda hard, resulting in us having to make a lot of
guesses...



What about a simple C extension, that could inspect those caches? 
Assuming it could be loaded into a single backend, that should be 
relatively acceptable way (compared to loading it to all backends using 
shared_preload_libraries).



--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[Andres Freund]

Hi,


On 2017-01-13 17:58:41 -0500, Tom Lane wrote:
> But, again, the catcache isn't the only source of per-process bloat
> and I'm not even sure it's the main one.  A more holistic approach
> might be called for.

It'd be helpful if we'd find a way to make it easy to get statistics
about the size of various caches in production systems. Right now that's
kinda hard, resulting in us having to make a lot of guesses...

Andres


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

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[Tom Lane]

Michael Paquier  writes:
> ... There are even some apps that do not use pgbouncer, but drop
> sessions after a timeout of inactivity to avoid a memory bloat because
> of the problem of this thread.

Yeah, a certain company I used to work for had to do that, though their
problem had more to do with bloat in plpgsql's compiled-functions cache
(and ensuing bloat in the plancache), I believe.

Still, I'm pretty suspicious of anything that will add overhead to
catcache lookups.  If you think the performance of those is not absolutely
critical, turning off the caches via -DCLOBBER_CACHE_ALWAYS will soon
disabuse you of the error.

I'm inclined to think that a more profitable direction to look in is
finding a way to limit the cache size.  I know we got rid of exactly that
years ago, but the problems with it were (a) the mechanism was itself
pretty expensive --- a global-to-all-caches LRU list IIRC, and (b) there
wasn't a way to tune the limit.  Possibly somebody can think of some
cheaper, perhaps less precise way of aging out old entries.  As for
(b), this is the sort of problem we made GUCs for.

But, again, the catcache isn't the only source of per-process bloat
and I'm not even sure it's the main one.  A more holistic approach
might be called for.

regards, tom lane


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

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[Michael Paquier]

On Sat, Jan 14, 2017 at 12:32 AM, Robert Haas  wrote:
> On Fri, Jan 13, 2017 at 8:58 AM, Tom Lane  wrote:
>> Michael Paquier  writes:
>>> Have there been ever discussions about having catcache entries in a
>>> shared memory area? This does not sound much performance-wise, I am
>>> just wondering about the concept and I cannot find references to such
>>> discussions.
>>
>> I'm sure it's been discussed.  Offhand I remember the following issues:
>>
>> * A shared cache would create locking and contention overhead.
>>
>> * A shared cache would have a very hard size limit, at least if it's
>> in SysV-style shared memory (perhaps DSM would let us relax that).
>>
>> * Transactions that are doing DDL have a requirement for the catcache
>> to reflect changes that they've made locally but not yet committed,
>> so said changes mustn't be visible globally.
>>
>> You could possibly get around the third point with a local catcache that's
>> searched before the shared one, but tuning that to be performant sounds
>> like a mess.  Also, I'm not sure how such a structure could cope with
>> uncommitted deletions: delete A -> remove A from local catcache, but not
>> the shared one -> search for A in local catcache -> not found -> search
>> for A in shared catcache -> found -> oops.
>
> I think the first of those concerns is the key one.  If searching the
> system catalogs costs $100 and searching the private catcache costs
> $1, what's the cost of searching a hypothetical shared catcache?  If
> the answer is $80, it's not worth doing.  If the answer is $5, it's
> probably still not worth doing.  If the answer is $1.25, then it's
> probably worth investing some energy into trying to solve the other
> problems you list.  For some users, the memory cost of catcache and
> syscache entries multiplied by N backends are a very serious problem,
> so it would be nice to have some other options.  But we do so many
> syscache lookups that a shared cache won't be viable unless it's
> almost as fast as a backend-private cache, or at least that's my
> hunch.

Being able to switch from one mode to another would be interesting.
Applications using extensing DDLs that require to change the catcache
with an exclusive lock would clearly pay the lock contention cost, but
do you think that be really the case of a shared lock? A bunch of
applications that I work with deploy Postgres once, then don't change
the schema except when an upgrade happens. So that would be benefitial
for that. There are even some apps that do not use pgbouncer, but drop
sessions after a timeout of inactivity to avoid a memory bloat because
of the problem of this thread. That won't solve the problem of the
local catcache bloat, but some users using few DDLs may be fine to pay
some extra concurrency cost if the session handling gets easied.

> I think it would be interested for somebody to build a prototype here
> that ignores all the problems but the first and uses some
> straightforward, relatively unoptimized locking strategy for the first
> problem.  Then benchmark it.  If the results show that the idea has
> legs, then we can try to figure out what a real implementation would
> look like.
> (One possible approach: use Thomas Munro's DHT stuff to build the shared 
> cache.)

Yeah, I'd bet on a couple of days of focus to sort that out.
-- 
Michael


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

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[Robert Haas]

On Fri, Jan 13, 2017 at 8:58 AM, Tom Lane  wrote:
> Michael Paquier  writes:
>> Have there been ever discussions about having catcache entries in a
>> shared memory area? This does not sound much performance-wise, I am
>> just wondering about the concept and I cannot find references to such
>> discussions.
>
> I'm sure it's been discussed.  Offhand I remember the following issues:
>
> * A shared cache would create locking and contention overhead.
>
> * A shared cache would have a very hard size limit, at least if it's
> in SysV-style shared memory (perhaps DSM would let us relax that).
>
> * Transactions that are doing DDL have a requirement for the catcache
> to reflect changes that they've made locally but not yet committed,
> so said changes mustn't be visible globally.
>
> You could possibly get around the third point with a local catcache that's
> searched before the shared one, but tuning that to be performant sounds
> like a mess.  Also, I'm not sure how such a structure could cope with
> uncommitted deletions: delete A -> remove A from local catcache, but not
> the shared one -> search for A in local catcache -> not found -> search
> for A in shared catcache -> found -> oops.

I think the first of those concerns is the key one.  If searching the
system catalogs costs $100 and searching the private catcache costs
$1, what's the cost of searching a hypothetical shared catcache?  If
the answer is $80, it's not worth doing.  If the answer is $5, it's
probably still not worth doing.  If the answer is $1.25, then it's
probably worth investing some energy into trying to solve the other
problems you list.  For some users, the memory cost of catcache and
syscache entries multiplied by N backends are a very serious problem,
so it would be nice to have some other options.  But we do so many
syscache lookups that a shared cache won't be viable unless it's
almost as fast as a backend-private cache, or at least that's my
hunch.

I think it would be interested for somebody to build a prototype here
that ignores all the problems but the first and uses some
straightforward, relatively unoptimized locking strategy for the first
problem.  Then benchmark it.  If the results show that the idea has
legs, then we can try to figure out what a real implementation would
look like.

(One possible approach: use Thomas Munro's DHT stuff to build the shared cache.)

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[Tom Lane]

Michael Paquier  writes:
> Have there been ever discussions about having catcache entries in a
> shared memory area? This does not sound much performance-wise, I am
> just wondering about the concept and I cannot find references to such
> discussions.

I'm sure it's been discussed.  Offhand I remember the following issues:

* A shared cache would create locking and contention overhead.

* A shared cache would have a very hard size limit, at least if it's
in SysV-style shared memory (perhaps DSM would let us relax that).

* Transactions that are doing DDL have a requirement for the catcache
to reflect changes that they've made locally but not yet committed,
so said changes mustn't be visible globally.

You could possibly get around the third point with a local catcache that's
searched before the shared one, but tuning that to be performant sounds
like a mess.  Also, I'm not sure how such a structure could cope with
uncommitted deletions: delete A -> remove A from local catcache, but not
the shared one -> search for A in local catcache -> not found -> search
for A in shared catcache -> found -> oops.

regards, tom lane


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

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[Michael Paquier]

On Wed, Dec 21, 2016 at 5:10 AM, Tom Lane  wrote:
> If I thought that "every ten minutes" was an ideal way to manage this,
> I might worry about that, but it doesn't really sound promising at all.
> Every so many queries would likely work better, or better yet make it
> self-adaptive depending on how much is in the local syscache.
>
> The bigger picture here though is that we used to have limits on syscache
> size, and we got rid of them (commit 8b9bc234a, see also
> https://www.postgresql.org/message-id/flat/5141.1150327541%40sss.pgh.pa.us)
> not only because of the problem you mentioned about performance falling
> off a cliff once the working-set size exceeded the arbitrary limit, but
> also because enforcing the limit added significant overhead --- and did so
> whether or not you got any benefit from it, ie even if the limit is never
> reached.  Maybe the present patch avoids imposing a pile of overhead in
> situations where no pruning is needed, but it doesn't really look very
> promising from that angle in a quick once-over.

Have there been ever discussions about having catcache entries in a
shared memory area? This does not sound much performance-wise, I am
just wondering about the concept and I cannot find references to such
discussions.
-- 
Michael


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

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[Kyotaro HORIGUCHI]

At Wed, 21 Dec 2016 10:21:09 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
 wrote in 
<20161221.102109.51106943.horiguchi.kyot...@lab.ntt.co.jp>
> At Tue, 20 Dec 2016 15:10:21 -0500, Tom Lane  wrote in 
> <23492.1482264...@sss.pgh.pa.us>
> > The bigger picture here though is that we used to have limits on syscache
> > size, and we got rid of them (commit 8b9bc234a, see also
> > https://www.postgresql.org/message-id/flat/5141.1150327541%40sss.pgh.pa.us)
> > not only because of the problem you mentioned about performance falling
> > off a cliff once the working-set size exceeded the arbitrary limit, but
> > also because enforcing the limit added significant overhead --- and did so
> > whether or not you got any benefit from it, ie even if the limit is never
> > reached.  Maybe the present patch avoids imposing a pile of overhead in
> > situations where no pruning is needed, but it doesn't really look very
> > promising from that angle in a quick once-over.
> 
> Indeed. As mentioned in the mail at the beginning of this thread,
> it hits the whole-cache scanning if at least one negative cache
> exists even it is not in a relation with the target relid, and it
> can be significantly long on a fat cache.
> 
> Lists of negative entries like CatCacheList would help but needs
> additional memeory.
> 
> > BTW, I don't see the point of the second patch at all?  Surely, if
> > an object is deleted or updated, we already have code that flushes
> > related catcache entries.  Otherwise the caches would deliver wrong
> > data.
> 
> Maybe you take the patch wrongly. Negative entires won't be
> flushed by any means. Deletion of a namespace causes cascaded
> object deletion according to dependency then finaly goes to
> non-neative cache invalidation. But a removal of *negative
> entries* in RELNAMENSP won't happen.
> 
> The test script for the case (gen2.pl) does the following thing,
> 
> CREATE SCHEMA foo;
> SELECT * FROM foo.invalid;
> DROP SCHEMA foo;
> 
> Removing the schema foo leaves a negative cache entry for
> 'foo.invalid' in RELNAMENSP.
> 
> However, I'm not sure the above situation happens so frequent
> that it is worthwhile to amend.

Since 1753b1b conflicts this patch, I rebased this onto the
current master HEAD. I'll register this to the next CF.

The points of discussion are the following, I think.

1. The first patch seems working well. It costs the time to scan
   the whole of a catcache that have negative entries for other
   reloids. However, such negative entries are created by rather
   unusual usages. Accesing to undefined columns, and accessing
   columns on which no statistics have created. The
   whole-catcache scan occurs on ATTNAME, ATTNUM and
   STATRELATTINH for every invalidation of a relcache entry.

2. The second patch also works, but flushing negative entries by
   hash values is inefficient. It scans the bucket corresponding
   to given hash value for OIDs, then flushing negative entries
   iterating over all the collected OIDs. So this costs more time
   than 1 and flushes involving entries that is not necessary to
   be removed. If this feature is valuable but such side effects
   are not acceptable, new invalidation category based on
   cacheid-oid pair would be needed.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
>From ee0cc13f70d79f23ec9507cf977228bba091bc49 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Thu, 15 Dec 2016 17:43:03 +0900
Subject: [PATCH 1/2] Cleanup negative cache of pg_statistic when dropping a
 relation.

Accessing columns that don't have statistics causes leaves negative
entries in catcache for pg_statstic, but there's no chance to remove
them. Especially when repeatedly creating then dropping temporary
tables bloats catcache so much that memory pressure can be
significant. This patch removes negative entries in STATRELATTINH,
ATTNAME and ATTNUM when corresponding relation is dropped.
---
 src/backend/utils/cache/catcache.c |  57 +++-
 src/backend/utils/cache/syscache.c | 277 +++--
 src/include/utils/catcache.h   |   3 +
 src/include/utils/syscache.h   |   2 +
 4 files changed, 265 insertions(+), 74 deletions(-)

diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c
index 6016d19..c1d9d2f 100644
--- a/src/backend/utils/cache/catcache.c
+++ b/src/backend/utils/cache/catcache.c
@@ -304,10 +304,11 @@ CatCachePrintStats(int code, Datum arg)
 
 		if (cache->cc_ntup == 0 && cache->cc_searches == 0)
 			continue;			/* don't print unused caches */
-		elog(DEBUG2, "catcache %s/%u: %d tup, %ld srch, %ld+%ld=%ld hits, %ld+%ld=%ld loads, %ld invals, %ld lsrch, %ld lhits",
+		elog(DEBUG2, "catcache %s/%u: %d tup, %d negtup, %ld srch, %ld+%ld=%ld hits, %ld+%ld=%ld loads, %ld invals, %ld lsrch, %ld lhits",
 			 cache->cc_relname,
 			 cache->cc_indexoid,
 			 cache->cc_ntup,
+			 

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[Kyotaro HORIGUCHI]

Thank you for the discussion.

At Tue, 20 Dec 2016 15:10:21 -0500, Tom Lane  wrote in 
<23492.1482264...@sss.pgh.pa.us>
> The bigger picture here though is that we used to have limits on syscache
> size, and we got rid of them (commit 8b9bc234a, see also
> https://www.postgresql.org/message-id/flat/5141.1150327541%40sss.pgh.pa.us)
> not only because of the problem you mentioned about performance falling
> off a cliff once the working-set size exceeded the arbitrary limit, but
> also because enforcing the limit added significant overhead --- and did so
> whether or not you got any benefit from it, ie even if the limit is never
> reached.  Maybe the present patch avoids imposing a pile of overhead in
> situations where no pruning is needed, but it doesn't really look very
> promising from that angle in a quick once-over.

Indeed. As mentioned in the mail at the beginning of this thread,
it hits the whole-cache scanning if at least one negative cache
exists even it is not in a relation with the target relid, and it
can be significantly long on a fat cache.

Lists of negative entries like CatCacheList would help but needs
additional memeory.

> BTW, I don't see the point of the second patch at all?  Surely, if
> an object is deleted or updated, we already have code that flushes
> related catcache entries.  Otherwise the caches would deliver wrong
> data.

Maybe you take the patch wrongly. Negative entires won't be
flushed by any means. Deletion of a namespace causes cascaded
object deletion according to dependency then finaly goes to
non-neative cache invalidation. But a removal of *negative
entries* in RELNAMENSP won't happen.

The test script for the case (gen2.pl) does the following thing,

CREATE SCHEMA foo;
SELECT * FROM foo.invalid;
DROP SCHEMA foo;

Removing the schema foo leaves a negative cache entry for
'foo.invalid' in RELNAMENSP.

However, I'm not sure the above situation happens so frequent
that it is worthwhile to amend.


regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[Robert Haas]

On Tue, Dec 20, 2016 at 3:10 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Tue, Dec 20, 2016 at 10:09 AM, Tom Lane  wrote:
>>> I don't understand why we'd make that a system-wide behavior at all,
>>> rather than expecting each process to manage its own cache.
>
>> Individual backends don't have a really great way to do time-based
>> stuff, do they?  I mean, yes, there is enable_timeout() and friends,
>> but I think that requires quite a bit of bookkeeping.
>
> If I thought that "every ten minutes" was an ideal way to manage this,
> I might worry about that, but it doesn't really sound promising at all.
> Every so many queries would likely work better, or better yet make it
> self-adaptive depending on how much is in the local syscache.

I don't think "every so many queries" is very promising at all.
First, it has the same problem as a fixed cap on the number of
entries: if you're doing a round-robin just slightly bigger than that
value, performance will be poor.  Second, what's really important here
is to keep the percentage of wall-clock time spent populating the
system caches small.  If a backend is doing 4000 queries/second and
each of those 4000 queries touches a different table, it really needs
a cache of at least 4000 entries or it will thrash and slow way down.
But if it's doing a query every 10 minutes and those queries
round-robin between 4000 different tables, it doesn't really need a
4000-entry cache.  If those queries are long-running, the time to
repopulate the cache will only be a tiny fraction of runtime.  If the
queries are short-running, then the effect is, percentage-wise, just
the same as for the high-volume system, but in practice it isn't
likely to be felt as much.  I mean, if we keep a bunch of old cache
entries around on a mostly-idle backend, they are going to be pushed
out of CPU caches and maybe even paged out.  One can't expect a
backend that is woken up after a long sleep to be quite as snappy as
one that's continuously active.

Which gets to my third point: anything that's based on number of
queries won't do anything to help the case where backends sometimes go
idle and sit there for long periods.  Reducing resource utilization in
that case would be beneficial.  Ideally I'd like to get rid of not
only the backend-local cache contents but the backend itself, but
that's a much harder project.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[Tom Lane]

Robert Haas  writes:
> On Tue, Dec 20, 2016 at 10:09 AM, Tom Lane  wrote:
>> I don't understand why we'd make that a system-wide behavior at all,
>> rather than expecting each process to manage its own cache.

> Individual backends don't have a really great way to do time-based
> stuff, do they?  I mean, yes, there is enable_timeout() and friends,
> but I think that requires quite a bit of bookkeeping.

If I thought that "every ten minutes" was an ideal way to manage this,
I might worry about that, but it doesn't really sound promising at all.
Every so many queries would likely work better, or better yet make it
self-adaptive depending on how much is in the local syscache.

The bigger picture here though is that we used to have limits on syscache
size, and we got rid of them (commit 8b9bc234a, see also
https://www.postgresql.org/message-id/flat/5141.1150327541%40sss.pgh.pa.us)
not only because of the problem you mentioned about performance falling
off a cliff once the working-set size exceeded the arbitrary limit, but
also because enforcing the limit added significant overhead --- and did so
whether or not you got any benefit from it, ie even if the limit is never
reached.  Maybe the present patch avoids imposing a pile of overhead in
situations where no pruning is needed, but it doesn't really look very
promising from that angle in a quick once-over.

BTW, I don't see the point of the second patch at all?  Surely, if
an object is deleted or updated, we already have code that flushes
related catcache entries.  Otherwise the caches would deliver wrong
data.

regards, tom lane


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

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[Robert Haas]

On Tue, Dec 20, 2016 at 10:09 AM, Tom Lane  wrote:
> Craig Ringer  writes:
>> On 20 December 2016 at 21:59, Robert Haas  wrote:
>>> We could implement this by having
>>> some process, like the background writer,
>>> SendProcSignal(PROCSIG_HOUSEKEEPING) to every process in the system
>>> every 10 minutes or so.
>
>> ... on a rolling basis.
>
> I don't understand why we'd make that a system-wide behavior at all,
> rather than expecting each process to manage its own cache.

Individual backends don't have a really great way to do time-based
stuff, do they?  I mean, yes, there is enable_timeout() and friends,
but I think that requires quite a bit of bookkeeping.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[Tom Lane]

Craig Ringer  writes:
> On 20 December 2016 at 21:59, Robert Haas  wrote:
>> We could implement this by having
>> some process, like the background writer,
>> SendProcSignal(PROCSIG_HOUSEKEEPING) to every process in the system
>> every 10 minutes or so.

> ... on a rolling basis.

I don't understand why we'd make that a system-wide behavior at all,
rather than expecting each process to manage its own cache.

regards, tom lane


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

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[Craig Ringer]

On 20 December 2016 at 21:59, Robert Haas  wrote:

> We could implement this by having
> some process, like the background writer,
> SendProcSignal(PROCSIG_HOUSEKEEPING) to every process in the system
> every 10 minutes or so.

... on a rolling basis.

Otherwise that'll be no fun at all, especially with some of those
lovely "we kept getting errors so we raised max_connections to 5000"
systems out there. But also on more sensibly configured ones that're
busy and want nice smooth performance without stalls.

-- 
 Craig Ringer   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

Re: [HACKERS] Protect syscache from bloating with negative cache entries

[Robert Haas]

On Mon, Dec 19, 2016 at 6:15 AM, Kyotaro HORIGUCHI
 wrote:
> Hello, recently one of my customer stumbled over an immoderate
> catcache bloat.

This isn't only an issue for negative catcache entries.  A long time
ago, there was a limit on the size of the relcache, which was removed
because if you have a workload where the working set of relations is
just larger than the limit, performance is terrible.  But the problem
now is that backend memory usage can grow without bound, and that's
also bad, especially on systems with hundreds of long-lived backends.
In connection-pooling environments, the problem is worse, because
every connection in the pool eventually caches references to
everything of interest to any client.

Your patches seem to me to have some merit, but I wonder if we should
also consider having a time-based threshold of some kind.  If, say, a
backend hasn't accessed a catcache or relcache entry for many minutes,
it becomes eligible to be flushed.  We could implement this by having
some process, like the background writer,
SendProcSignal(PROCSIG_HOUSEKEEPING) to every process in the system
every 10 minutes or so.  When a process receives this signal, it sets
a flag that is checked before going idle.  When it sees the flag set,
it makes a pass over every catcache and relcache entry.  All the ones
that are unmarked get marked, and all of the ones that are marked get
removed.  Access to an entry clears any mark.  So anything that's not
touched for more than 10 minutes starts dropping out of backend
caches.

Anyway, that would be a much bigger change from what you are proposing
here, and what you are proposing here seems reasonable so I guess I
shouldn't distract from it.  Your email just made me think of it,
because I agree that catcache/relcache bloat is a serious issue.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

[HACKERS] Protect syscache from bloating with negative cache entries

[Kyotaro HORIGUCHI]

Hello, recently one of my customer stumbled over an immoderate
catcache bloat.

This is a known issue living on the Todo page in the PostgreSQL
wiki.

https://wiki.postgresql.org/wiki/Todo#Cache_Usage
> Fix memory leak caused by negative catcache entries

https://www.postgresql.org/message-id/51c0a1ff.2050...@vmware.com

This patch addresses the two cases of syscache bloat by using
invalidation callback mechanism.


Overview of the patch

The bloat is caused by negative cache entries in catcaches. They
are crucial for performance but it is a problem that there's no
way to remove them. They last for the backends' lifetime.

The first patch provides a means to flush catcache negative
entries, then defines a relcache invalidation callback to flush
negative entries in syscaches for pg_statistic(STATRELATTINH) and
pg_attributes(ATTNAME, ATTNUM).  The second patch implements a
syscache invalidation callback so that deletion of a schema
causes a flush for pg_class (RELNAMENSP).

Both of the aboves are not hard-coded and defined in cacheinfo
using additional four members.


Remaining problems

Still, catcache can bloat by repeatedly accessing non-existent
table with unique names in a permanently-living schema but it
seems a bit too artificial (or malicious). Since such negative
entries don't have a trigger to remove, caps are needed to
prevent them from bloating syscaches, but the limits are hardly
seem reasonably determinable.


Defects or disadvantages

This patch scans over whole the target catcache to find negative
entries to remove and it might take a (comparably) long time on a
catcache with so many entries. By the second patch, unrelated
negative caches may be involved in flushing since they are keyd
by hashvalue, not by the exact key values.



The attached files are the following.

1. 0001-Cleanup-negative-cache-of-pg_statistic-when-dropping.patch
   Negative entry flushing by relcache invalidation using
   relcache invalidation callback.

2. 0002-Cleanup-negative-cache-of-pg_class-when-dropping-a-s.patch
   Negative entry flushing by catcache invalidation using
   catcache invalidation callback.

3. gen.pl
   a test script for STATRELATTINH bloating.

4. gen2.pl
   a test script for RELNAMENSP bloating.

3 and 4 are used as the following,

./gen.pl | psql postgres > /dev/null



regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
>From 203735e13d2b8584e1ddc652b602465a4f839355 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Thu, 15 Dec 2016 17:43:03 +0900
Subject: [PATCH 1/2] Cleanup negative cache of pg_statistic when dropping a
 relation.

Accessing columns that don't have statistics causes leaves negative
entries in catcache for pg_statstic, but there's no chance to remove
them. Especially when repeatedly creating then dropping temporary
tables bloats catcache so much that memory pressure can be
significant. This patch removes negative entries in STATRELATTINH,
ATTNAME and ATTNUM when corresponding relation is dropped.
---
 src/backend/utils/cache/catcache.c |  57 +++-
 src/backend/utils/cache/syscache.c | 274 +++--
 src/include/utils/catcache.h   |   3 +
 src/include/utils/syscache.h   |   2 +
 4 files changed, 263 insertions(+), 73 deletions(-)

diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c
index 6016d19..c1d9d2f 100644
--- a/src/backend/utils/cache/catcache.c
+++ b/src/backend/utils/cache/catcache.c
@@ -304,10 +304,11 @@ CatCachePrintStats(int code, Datum arg)
 
 		if (cache->cc_ntup == 0 && cache->cc_searches == 0)
 			continue;			/* don't print unused caches */
-		elog(DEBUG2, "catcache %s/%u: %d tup, %ld srch, %ld+%ld=%ld hits, %ld+%ld=%ld loads, %ld invals, %ld lsrch, %ld lhits",
+		elog(DEBUG2, "catcache %s/%u: %d tup, %d negtup, %ld srch, %ld+%ld=%ld hits, %ld+%ld=%ld loads, %ld invals, %ld lsrch, %ld lhits",
 			 cache->cc_relname,
 			 cache->cc_indexoid,
 			 cache->cc_ntup,
+			 cache->cc_nnegtup,
 			 cache->cc_searches,
 			 cache->cc_hits,
 			 cache->cc_neg_hits,
@@ -374,6 +375,10 @@ CatCacheRemoveCTup(CatCache *cache, CatCTup *ct)
 	/* free associated tuple data */
 	if (ct->tuple.t_data != NULL)
 		pfree(ct->tuple.t_data);
+
+	if (ct->negative)
+		--cache->cc_nnegtup;
+
 	pfree(ct);
 
 	--cache->cc_ntup;
@@ -637,6 +642,49 @@ ResetCatalogCache(CatCache *cache)
 }
 
 /*
+ *		CleanupCatCacheNegEntries
+ *
+ *	Remove negative cache tuples maching a partial key.
+ *
+ */
+void
+CleanupCatCacheNegEntries(CatCache *cache, ScanKeyData *skey)
+{
+	int i;
+
+	/* If this cache has no negative entries, nothing to do */
+	if (cache->cc_nnegtup == 0)
+		return;
+
+	/* searching with a paritial key needs scanning the whole cache */
+	for (i = 0; i < cache->cc_nbuckets; i++)
+	{
+		dlist_head *bucket = >cc_bucket[i];
+		dlist_mutable_iter iter;
+
+		dlist_foreach_modify(iter, bucket)
+		{
+			CatCTup*ct = dlist_container(CatCTup, cache_elem, iter.cur);
+			bool