| From: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> |
|---|---|
| To: | Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com> |
| Cc: | David Rowley <dgrowleyml(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Andres Freund <andres(at)anarazel(dot)de> |
| Subject: | Re: Reducing relcache memory usage: deduping index shapes |
| Date: | 2026-09-01 06:01:16 |
| Message-ID: | D5FC8DF7-0A90-4417-AF1C-F68BEDA81FBC@gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
> On Sep 1, 2026, at 07: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:
>>
>> On Tue, 1 Sept 2026 at 08:57, Matthias van de Meent
>> <boekewurm+postgres(at)gmail(dot)com> wrote:
>>> Additionally, it includes a patch by Andres (polished by me) that adds
>>> a proxy context, which reduces the the overhead of small and
>>> long-lived allocations in (what we expect to be) small memory contexts
>>> by forwarding the allocations to malloc (after wrapping the struct).
>>
>> Interesting. I looked at 0004 and expected that with a name like
>> "Proxy" that the allocation would be diverted to another context, such
>> as the parent context.
>
>
>
>> If all pallocs are going directly to malloc,
>> would "Direct" not be a more suitable name?
>
> That might be. Andres' patch named it Proxy, and I didn't have a
> sufficiently better name that would fit as alternative.
>
>> Also, just so it's written down somewhere, can you elaborate on the
>> choice not to have the code similar to as it is, but instead of malloc
>> directly with MemoryContextAlloc in the parent context? Is it just a
>> case of problems with double counting for the memory stats? Is there
>> some other reason why this would be bad?
>
> Allocating into another context has some big issues, and some smaller
> but also important issues:
> 1. Memory lifetime management becomes more complicated, given that you
> have to forward memory context resets to the proxied contexts.
> Allocating into the parent is insufficient, as parent contexts are not
> constant for the lifetime of all contexts.
> 2. None of PG's own memory contexts (or, memory allocators) are
> optimized for packing small, long-lived, mixed-lifetime allocations.
> 3. Allocating into other contexts would likely cause us to double-count memory.
>
>> From a quick read of 0004:
>>
>> 1. The new context type should get a mention in
>> src/backend/utils/mmgr/README under "Alternative Memory Context
>> Implementations"
>
> Will adjust.
>
>> 2. I think it's worth expanding the following to maybe tag something
>> like "i.e. are directly malloc'ed" or "i.e one malloc per palloc" to
>> the end. The whole thing about other context types managing oversized
>> chunks and directly mallocing a block for them is really up to them.
>>
>> + * Proxy is a MemoryContext implementation designed for memory usages which
>> + * require their own memory context, but which generally have few allocations
>> + * that generally have a very long lifetime. Compared to ASet, every
>> + * allocation of a Proxy memory context gets an External chunk.
>
> Will do.
>
>> 3. I don't quite understand the following comment. IMO, there is no
>> initial block here. This is just the malloc for the context struct
>> itself.
>>
>> + /*
>> + * Allocate the initial block. Unlike other proxy.c blocks, it starts
>> + * with the context header and its block header follows that.
>> + */
>
> Yeah, that's a part I failed to polish. I'll adjust it in my next patch.
>
>> 4. Per the discussion in [1], I think the preference is to use size_t
>> instead of Size.
>
> Yes, much of the patch is from a WIP patch of Andres'. I'd hoped I
> polished all rough edges, but I didn't get as far as I'd hoped before
> the deadline of the September commitfest start.
>
>> + Size totalspace;
>> + Size nchunks = 0;
>>
>> 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.
>
>> 5. The following WARNING looks buggy:
>>
>> + if (total_allocated != ctx->header.mem_allocated)
>> + {
>> + elog(WARNING, "problem in Proxy %s: amount of memory allocated %d
>> does not match header %d",
>> + name, (int) total_allocated, ctx->chunks_allocated);
>> + }
>>
>> Why cast to int?
>
> I've had some trouble finding the right format specifier, so cast to
> int was a simple hack on that.
>
>>> Earlier versions of the patch adjusted aset.c to accept smaller memory
>>> context sizes, but I abandoned that approach in favour of Andres'
>>> ProxyContext -- it can outsource most the complexities of memory
>>> management to the system allocator.
>>
>> Can you share more about this choice? What are the advantages of this
>> new context type over doing something like modifying aset.c to allow
>> passing of a 0 maxBlockSize so that all chunks are external?
>
> The AllocSet context requires a relatively large allocation, with
> larger overheads than the Proxy context (200B vs 108B). For the
> contexts I'm interested in (the relcache 'index info' contexts) this
> is a very significant difference: An aset context would increase
> memory usage of the contexts by ~35% (assuming initdb's catalogs are
> is a good sample).
>
> In this patch, the average 'index info' context uses about 329 bytes
> of memory (vs 2086 bytes on master). If an aset context was used,
> with its larger context and block structs, that average would be 447
> bytes.
>
>> If it
>> were done that way, the while loop at the end of
>> AllocSetContextCreateInternal() could calculate allocChunkLimit to be
>> 0 and that would result in AllocSetAlloc() always going with the
>> AllocSetAllocLarge() path. I currently can't see beyond this only
>> saving the "if (size > set->allocChunkLimit)" precheck. Or is it a
>> case of AllocSetContext being overly large due to the freelist array?
>
> Exactly, AllocSet adds significantly more overhead than Proxy does;
> the context itself is 85% larger, and its Block is 25% larger than its
> ProxyContext equivalent ProxyChunk.
>
>> Can you provide information about how much memory is being saved from 0004+0005?
>
> Attached the output for a query on catcache/relcache memory context
> data, with output of master and the full patchset.
>
> From Master to 0003, we go from 287872 bytes total to 141312 bytes
> total spent on "index info" contexts (some of which moved into other
> contexts, some new, but a net saving of 130kB). 0004+0005 bring that
> down all the way to 45392 bytes; most of which is just avoiding the
> large overhead of the pre-allocated Blocks of memory, saving another
> 95kB.
>
> Kind regards,
>
> Matthias van de Meent
> Databricks (https://www.databricks.com)
> <scratch_61.txt>
Hi Matthias,
Thanks for the patch, the idea is interesting. I have just gone through the commits, and got a suspicion.
Basically, the idea is to share common data via a hash table in each backend process, thereby saving some memory. However, sharing rd_supportinfo seems unsafe. In index_getprocinfo(), an entry is initialized lazily by calling fmgr_info_cxt(procId, locinfo, irel->rd_indexcxt);
This stores the current index's private context in locinfo->fn_mcxt. Support functions may then allocate fn_extra in that context. If this index's relcache entry is destroyed while another index still references the shared rd_supportinfo, the first index's rd_indexcxt is deleted, leaving the shared FmgrInfo with a dangling fn_mcxt and possibly a dangling fn_extra.
Am I missing something that guarantees the original rd_indexcxt remains valid for as long as the shared rd_supportinfo is referenced?
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
| From | Date | Subject | |
|---|---|---|---|
| Next Message | vignesh C | 2026-09-01 06:06:04 | Re: Logical replication row filter loses unchanged toasted columns |
| Previous Message | Michael Paquier | 2026-09-01 06:00:33 | Re: Routed ON CONFLICT inserts broken by partition-local deferrable unique constraints in 19 and master |