Re: Reducing relcache memory usage: deduping index shapes

On Tue, 1 Sept 2026 at 09:18, David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
>
> On Tue, 1 Sept 2026 at 11:10, Matthias van de Meent
> <boekewurm+postgres(at)gmail(dot)com> wrote:
> >
> > On Tue, 1 Sept 2026 at 00:04, David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
> > > Shouldn't nchunks be uint64 anyway? Nothing guarantees Size is bigger
> > > than int, even on 64-bit.
> >
> > True. But AFAIK, Size must be able to contain the largest possible
> > pointer difference, and that implies that we can't have more than
> > SIZE_MAX allocated chunks. Using Size for maths in those cases seems
> > appropriate, to avoid requiring expensive oversized registers on
> > 32-bit builds.
>
> Pointer difference relates to the size of a chunk. We're talking about
> the number of chunks. If a 64-bit platform exists with a 32-bit
> size_t, and you create > 2^32 chunks, then the counter will wrap and
> you'll mistakenly raise a WARNING.

If size_t is 32-bit, then that means all possible object sizes
(including array sizes) for the platform fit in those 32 bits. I'm
fairly sure that we don't support 64-bit addressing platforms that
don't also support objects larger than 2^32, though I'm happy to be
pointed to a (supported!) counterexample.

> I also think it's better to just remove the chunks_allocated struct
> field. It's not testing any code that exists in
> non-MEMORY_CONTEXT_CHECKING. All that's happening is you're adding a
> bunch of new code in MEMORY_CONTEXT_CHECKING builds and verifying that
> new code is ok. If you didn't add it, you wouldn't need to check it.

I used it to verify that the linked list is maintained correctly. I'd
have used dclist instead if it was simple to switch to that
implementation in only MEMORY_CONTEXT_TRACKING mode.

> More reviewing of 0004:
>
> 1. These Asserts also seem strange as they check something that's
> already been checked:
>
> + Assert(total_allocated == context->mem_allocated);
> + Assert(chunks_allocated == ctx->chunks_allocated);

I've adjusted this.

> 2. I think ExternalChunkGetBlock should be called something else as it
> returns a pointer to a ProxyChunk. Maybe MemoryChunkGetProxyChunk? You
> should also document what the parameter is to that macro.

I've adjusted the name of the struct to match its role -- it's more
comparable to aset's Block, so I updated its name.

> 3. Is "#include <limits.h>" just for INT_MAX? Normally we'd use
> PG_INT32_MAX from c.h.

I've seen various uses of INT_MAX from <limits.h> directly around the
codebase, so I don't see much of an issue with it. But, in v2 this
include has become redundant I reworked the code a bit in v2, and now
this check isn't present anymore.

> 4. If you are keen to save more memory, you could move away from using
> MemoryChunk and write your own version that maintains the lower 4-bits
> for the MemoryContextMethodID and encodes the size in the remaining 60
> bits. That might be more trouble than it's worth, however.

I don't think that exact scheme is allowed, because the most bits you
can consume is 59: Four bits are used by the MemoryContextMethodID,
and one bit stores the "external" bit. But when the external bit is
set, mctx infra expects these 59 bits to contain MEMORYCHUNK_MAGIC;
and when the external bit isn't set, it should contain a valid
`length` field less than .
And yes, I think that'd be more trouble than it's worth.

> 5. It would be good to see proxy.c using set_sentinel() and sentinel_ok(),

Added.

-------

Attached is v2 of the patchset. Changelist below:

0001/0002: Unchanged.

0003 (Deduplication):
* New memory context "Relation shape cache" to hold all shape-related
allocations and contexts,
This is a child context under CacheMemoryContext
* Memory context per "relation shape" in the RelShapeHash
This allows faster freeing of all associated data
* Optimized default shape entry data allocations per shape
The minimum is now down to 2 allocations, from >3. This is
primarily useful once Proxy contexts are used; Key data is still
bulk-allocated.
* Some varlena macro-related fixes.
Some SIZE/SIZE_EXHDR confusion and related issues, identified by
the sanitizer CF builds.

0004 (Proxy context):
* Consistency checks have been adjusted, and sentinel checks have been
introduced.
* Code has been updated with aset as template for naming and flow
This should clean up David's comments.

Name change from Proxy to anything else gets a 0-vote from me: I'd
like to avoid the churn, but if people have strong feelings about it
I'll go through the motions.

0005 (Apply proxy):
* Added Proxy to the new per-"index shape" contexts.

Question for the crowd: Most memory contexts often get a text
identifier which describes their contents in more detail when we have
many of the same name. "index shape" contexts don't have a simple
natural identifier. Whilst they do have the shape key, formatting
that into a name would be a bit of effort (and quite a bit of effort
if we want to capture the whole key), and we'd spend more bytes per
index shape. Do we want/need this identifier even with the increase
in memory usage?

Kind regards,

Matthias van de Meent
Databricks (https://www.databricks.com)