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

On Mon, 5 Oct 2026 at 13:48, David Geier <geidav(dot)pg(at)gmail(dot)com> wrote:
>
> I've also attached v3 which is v2 rebased on latest master. No other
> changes.

Some high-level comments:

0003-0008: These need more commit description than they currently have.

0001:
Could you replace the conditional initializations with Assert()s that
the context is initialized? That'll save any code that calls into
these functions too early.

0002: LGTM, though maybe an AllocRelationData() helper macro would
reduce the verbosity of each allocation, given that we're always
allocating into that one context.

0004:
I think this needs to introduce a field at the top level of
RelationData that indicates the variant of Relation that we're dealing
with. Continuously dereferencing rel->rd_rel->relkind seems rather
expensive compared to "just" keeping track of another boolean (or
enum, char, or whatever type) at the front of the struct that
distinguishes the exact type of data contained, if we can afford to.

(It'll also make it easier to verify we're dealing with the right type
of Relation, as we won't have to Assert() two layers of indirection
deep)

> +++ relcache.c

I think this'll be better if it splits out the Table and Index parts
from RelationBuildDesc.

> +/*
> + * Field order within RelationData (and within each branch of the union
> + * further down) is chosen to minimize struct padding: pointer-sized
> + * (8-byte) fields are grouped first, followed by 4-byte fields, followed
> + * by 1-byte bool fields. Don't reorder fields without re-checking that
> + * this packing is preserved.
> + */

I don't think this packing is necessarily better than "just" making
sure we order our fields in a way that introduces the fewest padding
bytes possible. Also note that the patch doesn't actually group all
fields in that order, so the comment is already outdated.

0005:

The following change snippet:

> + * RelationGetLockRelId() derives the LockRelId to use for locking the
> + * relation from rd_id and rd_rel->relisshared, in case anything tries
> + * to lock the dummy relation. We don't have a real OID for this fake
> + * entry, so use relNumber instead. Note that this is fairly bogus
> + * since relNumber may be different from the relation's real OID. It
> + * shouldn't really matter though. In recovery, we are running by
> + * ourselves and can't have any lock conflicts. While syncing, we
> + * already hold AccessExclusiveLock. Set relisshared to match rlocator
> + * so that RelationGetLockRelId() derives the correct dbId for shared
> + * relations (InvalidOid); for non-shared relations it'll derive
> + * MyDatabaseId, which is correct whenever this is called from a real
> + * backend (e.g. while syncing) and simply irrelevant during recovery.
> */
> - rel->rd_lockInfo.lockRelId.dbId = rlocator.dbOid;
> - rel->rd_lockInfo.lockRelId.relId = rlocator.relNumber;
> + rel->rd_id = rlocator.relNumber;
> + rel->rd_rel->relisshared = (rlocator.spcOid == GLOBALTABLESPACE_OID);

Shouldn't this test on rlocator.dbOid, or Assert() that the dbOid is
invalid when spcOid == GLOBALTABLESPACE_OID? Asking, because the lock
code uses dbOid.

0006:

In RelationGetPartitionDesc(), and various other functions, it may
make sense to store rd_partdesc in a local variable, it'll save line
width whenever we're frequently checking its data.

0007: -1 on this patch; I don't think the cost/benefit of this change
in ergonomics is worth it.

0008: -1 on this patch, too: I don't think the byte savings are worth
these changes; especially given that this adds additional indirections
with more data dependencies on common paths.

It also adds a 2k line propgraphcmds.c without adding it to compile
targets for it in Makefile nor meson.build, was this a typo?

Kind regards,

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