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

On Tue, Oct 13, 2026 at 11:09 AM Matthias van de Meent
<boekewurm+postgres(at)gmail(dot)com> wrote:
>
> 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)
>
>

Hi David,
I tested v3 independently on Linux x86_64, based on 9dbc53591a2. All
eight attachments apply there without context changes. I concentrated
on v3 0001-0006, following the cost/benefit concerns already raised by
Matthias about 0007 and 0008.
I found a predicate-locking crash introduced by the union in 0004. The
predicate code uses rd_index (and, with 0008, RelationGetIndex()) to
decide whether a Relation is an index. For a rule-bearing table,
rd_rules occupies the same union storage, so that test can interpret a
RuleLock as index metadata.
This reproduces with 0001-0006; 0008 is not required. A minimal case is:

CREATE TABLE t (a int PRIMARY KEY);
CREATE RULE r AS ON INSERT TO t DO ALSO SELECT 1;
BEGIN ISOLATION LEVEL SERIALIZABLE;
INSERT INTO t VALUES (1);
TRUNCATE t;
In a cassert build, TRUNCATE aborts at
CheckTableForSerializableConflictIn(). At that point relkind is 'r',
and rd_rules and the overlaid rd_index contain the same non-NULL address.
The server process exits with SIGABRT. The unpatched baseline passes the
same pg_regress test.
The first attached patch adds RelationIsIndex(), based on relkind, and
uses it at the three predicate-locking classification sites. The second
adds the pg_regress test. Both ordinary and partitioned indexes remain
covered by the predicate.

I also reproduced an ERROR-path problem from 0006 using an injection
point at its new RelationPartitionInfo allocation inside
RelationRebuildRelation(). After an ALTER TABLE added a column to a
partitioned table, the injected ERROR occurred after the descriptor swap.
Rollback restored the two-column catalog state, but the same backend kept
the three-column descriptor and then reported:

could not find inherited attribute "key_added"

An equivalent general swap window exists without this series. The v3
regression is specifically that 0006 adds an allocation failure point to
the region. I did not observe or claim a real OOM.

The third patch allocates destination partition metadata before the swap
and before reparenting a completed partition-key context. It also avoids
reading table-side partition metadata for index relations. With the
allocation moved before the swap, an ERROR leaves the old descriptor intact
and marked invalid; the next lookup rebuilds it normally after rollback.
Ten repeated subtransaction failures, partitioned indexes, and
debug_discard_caches=1 also passed without an external invalidation.

Finally, the init-file loader reconstructs index access fields but does
not reset rd_indexprs, rd_indpred, rd_exclops, rd_exclprocs, or
rd_exclstrats. Those are serialized process pointers whose contents are
supposed to be initialized lazily. The fourth patch resets the five
fields without changing the init-file format. I did not find a reachable
failure with current catalog indexes, so I regard this as an
initialization correctness hazard rather than a demonstrated SQL crash.

On Linux, sizeof(RelationData) is 496 bytes at the baseline, 272 through
0006 and 0007, and 264 through 0008. Under the same 8 KiB Slab setup,
0006 and 0007 both used 302 blocks and 2,474,160 total bytes for a workload
loading 8,731 relations. This supports the limited-benefit point already
made in the review discussion.

One additional issue: original v3 0008 does not build on this base because
execReplication.c still has two rd_index references. I did not repair it
silently or use that configuration for runtime results.

Validation was on Linux 6.17 x86_64 with GCC 13.3 and Clang 18.1. GCC
cassert and release check-world passed (240 core regression and 133
isolation tests). Clang ASan/UBSan core and isolation suites also passed.

Attachments:

0001: classify indexes by relkind in predicate locking
0002: add the rule-bearing SERIALIZABLE regression test
0003: keep partition-info allocations outside descriptor swaps
0004: reset lazy index pointers after init-file loading

Regards,
Haibo