Обсуждение: Separate memory contexts for relcache and catcache

On 2023-Aug-09, Melih Mutlu wrote:

> --Patch
>          name          | used_bytes | free_bytes | total_bytes
> -----------------------+------------+------------+-------------
>  RelCacheMemoryContext |    4706464 |    3682144 |     8388608
>  CatCacheMemoryContext |    3489384 |     770712 |     4260096
>  index info            |    2102160 |     113776 |     2215936
>  CacheMemoryContext    |       2336 |       5856 |        8192
>  relation rules        |       4416 |       3776 |        8192
> (5 rows)

Hmm, is this saying that there's too much fragmentation in the relcache
context?  Maybe it would improve things to make it a SlabContext instead
of AllocSet.  Or, more precisely, a bunch of SlabContexts, each with the
appropriate chunkSize for the object being stored.  (I don't say this
because I know for a fact that Slab is better for these purposes; it's
just that I happened to read its comments yesterday and they stated that
it behaves better in terms of fragmentation.  Maybe Andres or Tomas have
an opinion on this.)

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"I love the Postgres community. It's all about doing things _properly_. :-)"
(David Garamond)

On Thu, 10 Aug 2023 at 01:23, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2023-Aug-09, Melih Mutlu wrote:
>
> > --Patch
> >          name          | used_bytes | free_bytes | total_bytes
> > -----------------------+------------+------------+-------------
> >  RelCacheMemoryContext |    4706464 |    3682144 |     8388608
> >  CatCacheMemoryContext |    3489384 |     770712 |     4260096
> >  index info            |    2102160 |     113776 |     2215936
> >  CacheMemoryContext    |       2336 |       5856 |        8192
> >  relation rules        |       4416 |       3776 |        8192
> > (5 rows)
>
> Hmm, is this saying that there's too much fragmentation in the relcache
> context?

free_bytes is just the space in the blocks that are not being used by
any allocated chunks or chunks on the freelist.

It looks like RelCacheMemoryContext has 10 blocks including the 8kb
initial block:

postgres=# select 8192 + sum(8192*power(2,x)) as total_bytes from
generate_series(0,9) x;
 total_bytes
-------------
     8388608

The first 2 blocks are 8KB as we only start doubling after we malloc
the first 8kb block after the keeper block.

If there was 1 fewer block then total_bytes would be 4194304, which is
less than the used_bytes for that context, so those 10 block look
needed.

> Maybe it would improve things to make it a SlabContext instead
> of AllocSet.  Or, more precisely, a bunch of SlabContexts, each with the
> appropriate chunkSize for the object being stored.

It would at least save from having to do the power of 2 rounding that
aset does. However, on a quick glance, it seems not all the size
requests in relcache.c are fixed.  I see a datumCopy() in
RelationBuildTupleDesc() for the attmissingval stuff, so we couldn't
SlabAlloc that.

It could be worth looking at the size classes of the fixed-sized
allocations to estimate how much memory we might save by using slab to
avoid the power-2 rounding that aset.c does. However, if there are too
many contexts then we may end up using more memory with all the
mostly-empty contexts for backends that only query a tiny number of
tables.  That might not be good.  Slab also does not do block doubling
like aset does, so it might be hard to choose a good block size.

> (I don't say this
> because I know for a fact that Slab is better for these purposes; it's
> just that I happened to read its comments yesterday and they stated that
> it behaves better in terms of fragmentation.  Maybe Andres or Tomas have
> an opinion on this.)

I'm not sure of the exact comment, but I was in the recently and
there's a chance that I wrote that comment.  Slab priorities putting
new chunks on fuller blocks and may free() blocks once they become
empty of any chunks.  Aset does no free()ing of blocks unless a block
was malloc()ed especially for a chunk above allocChunkLimit.  That
means aset might hold a lot of malloc'ed memory for chunks that just
sit on freelists which might never be used ever again, meanwhile,
other request sizes may have to malloc new blocks.

David

Hi,

On 2023-08-09 15:02:31 +0300, Melih Mutlu wrote:
> To quickly show how pg_backend_memory_contexts would look like, I did the
> following:
> 
> -Create some tables:
> SELECT 'BEGIN;' UNION ALL SELECT format('CREATE TABLE %1$s(id serial
> primary key, data text not null unique)', 'test_'||g.i) FROM
> generate_series(0, 1000) g(i) UNION ALL SELECT 'COMMIT;';\gexec
> 
> -Open a new connection and query pg_backend_memory_contexts [1]:
> This is what you'll see before and after the patch.
> -- HEAD:
>         name        | used_bytes | free_bytes | total_bytes
> --------------------+------------+------------+-------------
>  CacheMemoryContext |     467656 |      56632 |      524288
>  index info         |     111760 |      46960 |      158720
>  relation rules     |       4416 |       3776 |        8192
> (3 rows)
> 
> -- Patch:
>          name          | used_bytes | free_bytes | total_bytes
> -----------------------+------------+------------+-------------
>  CatCacheMemoryContext |     217696 |      44448 |      262144
>  RelCacheMemoryContext |     248264 |      13880 |      262144
>  index info            |     111760 |      46960 |      158720
>  CacheMemoryContext    |       2336 |       5856 |        8192
>  relation rules        |       4416 |       3776 |        8192
> (5 rows)

Have you checked what the source of the remaining allocations in
CacheMemoryContext are?


One thing that I had observed previously and reproduced with this patch, is
that the first backend starting after a restart uses considerably more memory:

first:
┌───────────────────────┬────────────┬────────────┬─────────────┐
│         name          │ used_bytes │ free_bytes │ total_bytes │
├───────────────────────┼────────────┼────────────┼─────────────┤
│ CatCacheMemoryContext │     370112 │     154176 │      524288 │
│ RelCacheMemoryContext │     244136 │      18008 │      262144 │
│ index info            │     104392 │      45112 │      149504 │
│ CacheMemoryContext    │       2304 │       5888 │        8192 │
│ relation rules        │       3856 │        240 │        4096 │
└───────────────────────┴────────────┴────────────┴─────────────┘

second:
┌───────────────────────┬────────────┬────────────┬─────────────┐
│         name          │ used_bytes │ free_bytes │ total_bytes │
├───────────────────────┼────────────┼────────────┼─────────────┤
│ CatCacheMemoryContext │     215072 │      47072 │      262144 │
│ RelCacheMemoryContext │     243856 │      18288 │      262144 │
│ index info            │     104944 │      47632 │      152576 │
│ CacheMemoryContext    │       2304 │       5888 │        8192 │
│ relation rules        │       3856 │        240 │        4096 │
└───────────────────────┴────────────┴────────────┴─────────────┘

This isn't caused by this patch, but it does make it easier to pinpoint than
before.  The reason is fairly simple: On the first start we start without
being able to use relcache init files, in later starts we can. The reason the
size increase is in CatCacheMemoryContext, rather than RelCacheMemoryContext,
is simple: When using the init file the catcache isn't used, when not, we have
to query the catcache a lot to build the initial relcache contents.


Given the size of both CatCacheMemoryContext and RelCacheMemoryContext in a
new backend, I think it might be worth using non-default aset parameters. A
bit ridiculous to increase block sizes from 8k upwards in every single
connection made to postgres ever.


> - Run select on all tables
> SELECT format('SELECT count(*) FROM %1$s', 'test_'||g.i) FROM
> generate_series(0, 1000) g(i);\gexec
> 
> - Then check pg_backend_memory_contexts [1] again:
> --HEAD
>         name        | used_bytes | free_bytes | total_bytes
> --------------------+------------+------------+-------------
>  CacheMemoryContext |    8197344 |     257056 |     8454400
>  index info         |    2102160 |     113776 |     2215936
>  relation rules     |       4416 |       3776 |        8192
> (3 rows)
> 
> --Patch
>          name          | used_bytes | free_bytes | total_bytes
> -----------------------+------------+------------+-------------
>  RelCacheMemoryContext |    4706464 |    3682144 |     8388608
>  CatCacheMemoryContext |    3489384 |     770712 |     4260096
>  index info            |    2102160 |     113776 |     2215936
>  CacheMemoryContext    |       2336 |       5856 |        8192
>  relation rules        |       4416 |       3776 |        8192
> (5 rows)
> 
> You can see that CacheMemoryContext does not use much memory without
> catcache and relcache (at least in cases similar to above), and it's easy
> to bloat catcache and relcache. That's why I think it would be useful to
> see their usage separately.

Yes, I think it'd be quite useful. There's ways to bloat particularly catcache
much further, and it's hard to differentiate that from other sources of bloat
right now.


> +static void
> +CreateCatCacheMemoryContext()

We typically use (void) to differentiate from an older way of function
declarations that didn't have argument types.


> +{
> +    if (!CacheMemoryContext)
> +        CreateCacheMemoryContext();

I wish we just made sure that cache memory context were created in the right
place, instead of spreading this check everywhere...


> @@ -3995,9 +3998,9 @@ RelationCacheInitializePhase2(void)
>          return;
>  
>      /*
> -     * switch to cache memory context
> +     * switch to relcache memory context
>       */
> -    oldcxt = MemoryContextSwitchTo(CacheMemoryContext);
> +    oldcxt = MemoryContextSwitchTo(RelCacheMemoryContext);
>  
>      /*
>       * Try to load the shared relcache cache file.  If unsuccessful, bootstrap
> @@ -4050,9 +4053,9 @@ RelationCacheInitializePhase3(void)
>      RelationMapInitializePhase3();
>  
>      /*
> -     * switch to cache memory context
> +     * switch to relcache memory context
>       */
> -    oldcxt = MemoryContextSwitchTo(CacheMemoryContext);
> +    oldcxt = MemoryContextSwitchTo(RelCacheMemoryContext);
>  
>      /*
>       * Try to load the local relcache cache file.  If unsuccessful, bootstrap

I'd just delete these comments, they're just pointlessly restating the code.


Greetings,

Andres Freund

Hi,

I also think this change would be helpful.

I imagine you're working on the Andres's comments and you already notice 
this, but v1 patch cannot be applied to HEAD.
For the convenience of other reviewers, I marked it 'Waiting on Author'.

-- 
Regards,

--
Atsushi Torikoshi
NTT DATA Group Corporation

On Wed, 3 Jan 2024 at 16:56, Melih Mutlu <m.melihmutlu@gmail.com> wrote:
>
> Hi,
>
> torikoshia <torikoshia@oss.nttdata.com>, 4 Ara 2023 Pzt, 07:59 tarihinde şunu yazdı:
>>
>> Hi,
>>
>> I also think this change would be helpful.
>>
>> I imagine you're working on the Andres's comments and you already notice
>> this, but v1 patch cannot be applied to HEAD.
>> For the convenience of other reviewers, I marked it 'Waiting on Author'.
>
>
> Thanks for letting me know. I rebased the patch. PFA new version.

CFBot shows that the patch does not apply anymore as in [1]:
=== Applying patches on top of PostgreSQL commit ID
729439607ad210dbb446e31754e8627d7e3f7dda ===
=== applying patch
./v2-0001-Separate-memory-contexts-for-relcache-and-catcach.patch
patching file src/backend/utils/cache/catcache.c
...
Hunk #8 FAILED at 1933.
Hunk #9 succeeded at 2253 (offset 84 lines).
1 out of 9 hunks FAILED -- saving rejects to file
src/backend/utils/cache/catcache.c.rej

Please post an updated version for the same.

[1] - http://cfbot.cputube.org/patch_46_4554.log

Regards,
Vignesh