Re: Reducing relcache memory usage 2: shrink sizeof(RelationData)

On Fri, 16 Oct 2026 at 15:02, David Geier <geidav(dot)pg(at)gmail(dot)com> wrote:
>
> Attached is v4 of the patch set.

0002:

Though I suggested the macro, I'm very much open to changing the name
to something better than AllocRelationData().

> @@ -6301,7 +6324,8 @@ load_relcache_init_file(bool shared)
> rels = repalloc_array(rels, Relation, max_rels);
> }
>
> - rel = rels[num_rels++] = (Relation) palloc(len);
> + rel = rels[num_rels++] = (Relation)
> + MemoryContextAlloc(RelationSlabContext, len);
>

This should probably check that len == sizeof(RelationData), and then
just use the AllocRelationData() macro used everywhere else.

0003: I agree with Alvaro's comment; if moving ForeignKeyCacheInfo to
its own header would also solve the issue then that's preferred.

0004:

> Table-like relations and indexes use disjoint sets of cached metadata. Put
> those fields in an anonymous union so each relcache entry pays only for the
> larger set instead of both sets.

I'd write that as

[...] pays only for the larger of both sets, instead of the sum of both sets.

> +++ b/src/backend/storage/lmgr/predicate.c
> @@ -2558,7 +2558,7 @@ PredicateLockTID(Relation relation, const ItemPointerData *tid, Snapshot snapsho
> /*
> * Return if this xact wrote it.
> */
> - if (relation->rd_index == NULL)
> + if (!RelationIsIndex(relation))

You mentioned that the additional lookup is minimal, but I have to
disagree here. In this case, non-indexes can go into a short path
that's now one indirection more expensive than before. All the
changes from rd_index checks to RelationIsIndex() with higher
indirections may also be a sign that this is a common pattern that's
become more expensive.

> - Assert(relation->rd_index == NULL); /* not an index relation */
> + Assert(!RelationIsIndex(relation));

Let's not drop the comments in the refactoring.

> @@ -2658,9 +2669,9 @@ RelationRebuildRelation(Relation relation)

I think this should be treated similar to
RelationBuildDesc/RelationDestroyRelation: Have a common path for
non-union fields, and then a single code block (if+else, or
switch+cases) for each of the union's variants.

0005:

I think maybe we should put RelationPartitionInfo into a Slab
allocator, too, or somehow figure out how to reduce its size. It's
currently 8 pointers, plus 5 bytes of scalar fields and 3 bytes of
alignment. That's just a bit larger than Aset's 64-byte bucket
boundary, and I'd hate to waste 56 bytes /allocation in highly
partitioned workloads.

0006:
By splitting enabled/valid -bools away from their pointers, we're
reducing locality of lookups by a small margin, making accesses on
just those fields slightly more expensive if they're not found on the
same cache line. I think the savings can still be worth it, but it's
something you may want to think about a bit more.

Nit:
> + bool pgstat_enabled; /* should relation stats be counted */
This is a pre-existing nit, but I'm not happy with the inconsistent
name of this field (and its sibling pgstat_info). All other fields are
rd_-prefixed, except these pgstat_-prefixed fields.

0007:
> +++ b/src/backend/access/nbtree/nbtutils.c
> +#include "storage/lmgr.h"

> +++ b/src/include/utils/rel.h
> +#include "miscadmin.h"

Why did we need add includes here, when both already should have
rel.h's full definitions included, and the changes are relevant only
for things that touch(ed) RelationData directly?

Kind regards,

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