Thread: parse_oper cache

On Sat, Nov 14, 2009 at 6:36 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> There's an example in parse_oper.c of a specialized cache that's about
> as complicated as this would be.

I was just taking a look at find_oper_cache_entry() and noticed
something odd.  When we discover that OprCacheHash == NULL, we make
sure that CacheMemoryContext is initialized before calling
hash_create().  But since we don't pass HASH_CONTEXT to hash_create(),
ISTM it's going to use TopMemoryContext anyhow.  utils/mmgr/README
indicates that the two contexts are basically equivalent anyway so I
don't think there's any visible breakage as a result of this, but it
sort of looks like an oversight.

...Robert

Robert Haas <robertmhaas@gmail.com> writes:
> I was just taking a look at find_oper_cache_entry() and noticed
> something odd.  When we discover that OprCacheHash == NULL, we make
> sure that CacheMemoryContext is initialized before calling
> hash_create().  But since we don't pass HASH_CONTEXT to hash_create(),
> ISTM it's going to use TopMemoryContext anyhow.  utils/mmgr/README
> indicates that the two contexts are basically equivalent anyway so I
> don't think there's any visible breakage as a result of this, but it
> sort of looks like an oversight.

Hmm.  Now that I look, I find half a dozen other places where a caller
of hash_create first calls CreateCacheMemoryContext but then doesn't
do anything with the context.  Some of them are very old code, like
relcache.c, and I'm sure the parse_oper case was copied from someplace
else rather than being coded from scratch.  I wonder whether this is
a leftover of a time when hash_create had a different default for where
to put hash tables.  Too lazy to dig in the CVS history to confirm that
thought though.

My inclination is to leave the actual memory allocation behavior alone
and just get rid of the CreateCacheMemoryContext calls in these places.
        regards, tom lane

On Sun, Dec 27, 2009 at 12:59 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> My inclination is to leave the actual memory allocation behavior alone
> and just get rid of the CreateCacheMemoryContext calls in these places.

I don't have a strong feeling on it.  It is only one extra line of
code to make the hash table context a child of CacheMemoryContext
rather than TopMemoryContext, but since we've been doing it this way
for so long, there's not any real clear value in changing it.  Though
it does make me wonder whether there's any point in retaining
CacheMemoryContext at all.

...Robert

Robert Haas <robertmhaas@gmail.com> writes:
> it does make me wonder whether there's any point in retaining
> CacheMemoryContext at all.

Well, as per backend/utils/mmgr/README:
 CacheMemoryContext --- permanent storage for relcache, catcache, and related modules.  This will never be reset or
deleted,either, so it's not truly necessary to distinguish it from TopMemoryContext.  But it seems worthwhile to
maintainthe distinction for debugging purposes.
 

I'm not sure about the word "maintain" here; I can't recall whether
there was any comparable concept before we invented the memory context
mechanism.  But I still think it's useful to distinguish cache activity
from generic permanent memory allocations.
        regards, tom lane

On Sun, Dec 27, 2009 at 2:45 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> it does make me wonder whether there's any point in retaining
>> CacheMemoryContext at all.
>
> Well, as per backend/utils/mmgr/README:
>
>  CacheMemoryContext --- permanent storage for relcache, catcache, and
>  related modules.  This will never be reset or deleted, either, so it's
>  not truly necessary to distinguish it from TopMemoryContext.  But it
>  seems worthwhile to maintain the distinction for debugging purposes.
>
> I'm not sure about the word "maintain" here; I can't recall whether
> there was any comparable concept before we invented the memory context
> mechanism.  But I still think it's useful to distinguish cache activity
> from generic permanent memory allocations.

If we're really doing it, sure.  But putting half of it in
TopMemoryContext and the other half in CacheMemoryContext is not
obviously of any value.

...Robert

Robert Haas <robertmhaas@gmail.com> writes:
> If we're really doing it, sure.  But putting half of it in
> TopMemoryContext and the other half in CacheMemoryContext is not
> obviously of any value.

There isn't any of that stuff that's *in* TopMemoryContext.  Whether the
hash table contexts are children of TopMemoryContext or
CacheMemoryContext would be important if we were ever going to reset
either, but we aren't.  The main point in my mind is that it be possible
to tell from a memory stats dump how much is being used for what, and we
do have that.
        regards, tom lane

On Sun, Dec 27, 2009 at 3:03 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> If we're really doing it, sure.  But putting half of it in
>> TopMemoryContext and the other half in CacheMemoryContext is not
>> obviously of any value.
>
> There isn't any of that stuff that's *in* TopMemoryContext.  Whether the
> hash table contexts are children of TopMemoryContext or
> CacheMemoryContext would be important if we were ever going to reset
> either, but we aren't.  The main point in my mind is that it be possible
> to tell from a memory stats dump how much is being used for what, and we
> do have that.

Oh, I see.  I was thinking that it might matter that the hash table
contexts are descended from TopMemoryContext rather than
CacheMemoryContext, but I guess that doesn't matter very much.

...Robert