RE: Protect syscache from bloating with negative cache entries

Hi Horiguchi-san,

I've looked through your patches. This is the first part of my review results. Let me post the rest after another work today.

BTW, how about merging 0003 and 0005, and separating and deferring 0004 in another thread? That may help to relieve other community members by making this patch set not so large and complex.

[Bottleneck investigation]
Ideriha-san and I are trying to find the bottleneck. My first try shows there's little overhead. Here's what I did:

<postgresql.conf>
shared_buffers = 1GB
catalog_cache_prune_min_age = -1
catalog_cache_max_size = 10MB

<benchmark>
$ pgbench -i -s 10
$ pg_ctl stop and then start
$ cache all data in shared buffers by running pg_prewarm on branches, tellers, accounts, and their indexes
$ pgbench --select-only -c 1 -T 60

<result>
master : 8612 tps
patched: 8553 tps (-0.7%)

There's little (0.7%) performance overhead with:
* one additional dlist_move_tail() in every catcache access
* memory usage accounting in operations other than catcache access (relevant catcache entries should be cached in the first pgbench transaction)

I'll check other patterns to find out how big overhead there is.

[Source code review]
Below are my findings on the patch set v15:

(1) patch 0001
All right.

(2) patch 0002
@@ -87,6 +87,7 @@ typedef struct MemoryContextData
const char *name; /* context name (just for debugging) */
const char *ident; /* context ID if any (just for debugging) */
MemoryContextCallback *reset_cbs; /* list of reset/delete callbacks */
+ uint64 consumption; /* accumulates consumed memory size */
} MemoryContextData;

Size is more appropriate as a data type than uint64 because other places use Size for memory size variables.

How about "usedspace" instead of "consumption"? Because that aligns better with the naming used for MemoryContextCounters's member variables, totalspace and freespace.

(3) patch 0002
+ context->consumption += chunk_size;
(and similar sites)

The used space should include the size of the context-type-specific chunk header, so that the count is closer to the actual memory size seen by the user.

Here, let's make consensus on what the used space represents. Is it either of the following?

a) The total space allocated from OS. i.e., the sum of the malloc()ed regions for a given memory context.
b) The total space of all chunks, including their headers, of a given memory context.

a) is better because that's the actual memory usage from the DBA's standpoint. But a) cannot be used because CacheMemoryContext is used for various things. So we have to compromise on b). Is this OK?

One possible future improvement is to use a separate memory context exclusively for the catcache, which is a child of CacheMemoryContext. That way, we can adopt a).

(4) patch 0002
@@ -614,6 +614,9 @@ AllocSetReset(MemoryContext context)
+ set->header.consumption = 0;

This can be put in MemoryContextResetOnly() instead of context-type-specific reset functions.

Regards
Takayuki Tsunakawa