On Fri, 16 Oct 2026 at 15:02, David Geier <[email protected]> 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)
