On Tue, Oct 13, 2026 at 11:09 AM Matthias van de Meent
<[email protected]> wrote:
>
> On Mon, 5 Oct 2026 at 13:48, David Geier <[email protected]> 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
From: PostgreSQL relcache v3 Linux review
Subject: [PATCH 4/4] Reset lazy index pointers after init-file load
RelationData is copied verbatim to relcache init files, but index expression,
predicate, and exclusion metadata are intentionally rebuilt lazily. Reset
the five corresponding pointers after loading the serialized index data so
stale process addresses cannot survive into a new backend.
Depends on patch 1 and applies after David Geier's v3 patches 0001 through
0006, based on 9dbc53591a2de03e0cfdab61a78726f8b2c29eef. The serialized
format is unchanged.
---
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index c4b7e01503b..c4192b255ef 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -6570,6 +6570,13 @@ load_relcache_init_file(bool shared)
nsupport = relform->relnatts * rel->rd_indam->amsupport;
rel->rd_supportinfo = (FmgrInfo *)
MemoryContextAllocZero(indexcxt, nsupport * sizeof(FmgrInfo));
+
+ /* These lazily built fields are not stored in the init file. */
+ rel->rd_indexprs = NIL;
+ rel->rd_indpred = NIL;
+ rel->rd_exclops = NULL;
+ rel->rd_exclprocs = NULL;
+ rel->rd_exclstrats = NULL;
}
else
{
From: PostgreSQL relcache v3 Linux review
Subject: [PATCH 1/4] Use relkind to classify relations in predicate locking
RelationData overlays table rewrite rules with index metadata. Testing the
overlaid pointer therefore misclassifies rule-bearing tables as indexes and
can dereference a RuleLock as Form_pg_index. Classify ordinary and
partitioned indexes by relkind instead.
Applies after David Geier's v3 patches 0001 through 0006, based on
9dbc53591a2de03e0cfdab61a78726f8b2c29eef.
---
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index 0ae85b7d5b4..5c539306e77 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ 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))
{
/* If we wrote it; we already have a write lock. */
if (TransactionIdIsCurrentTransactionId(tuple_xid))
@@ -2890,7 +2890,7 @@ DropAllPredicateLocksFromTable(Relation relation, bool transfer)
dbId = relation->rd_locator.dbOid;
relId = relation->rd_id;
- if (relation->rd_index == NULL)
+ if (!RelationIsIndex(relation))
{
isIndex = false;
heapId = relId;
@@ -4371,7 +4371,7 @@ CheckTableForSerializableConflictIn(Relation relation)
*/
MyXactDidWrite = true;
- Assert(relation->rd_index == NULL); /* not an index relation */
+ Assert(!RelationIsIndex(relation));
dbId = relation->rd_locator.dbOid;
heapId = relation->rd_id;
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index 33a3921f5cf..60db9fd8753 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -309,6 +309,11 @@ typedef struct ViewOptions
*/
#define RelationGetRelid(relation) ((relation)->rd_id)
+/* True for both ordinary and partitioned index relations. */
+#define RelationIsIndex(relation) \
+ ((relation)->rd_rel->relkind == RELKIND_INDEX || \
+ (relation)->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
+
/*
* RelationGetLockRelId
* Returns the LockRelId to use for locking the relation.
From: PostgreSQL relcache v3 Linux review
Subject: [PATCH 3/4] Keep allocations outside relcache descriptor swaps
Allocate RelationPartitionInfo before reparenting a completed partition-key
context and before beginning RelationRebuildRelation's descriptor swap. An
allocation ERROR inside that swap can otherwise leave a valid relcache entry
with a descriptor from an aborted catalog change.
Guard the table-side union access by relation kind. Depends on patch 1 and
applies after David Geier's v3 patches 0001 through 0006, based on
9dbc53591a2de03e0cfdab61a78726f8b2c29eef.
---
diff --git a/src/backend/utils/cache/partcache.c b/src/backend/utils/cache/partcache.c
index 959ac904bf7..05964951cd8 100644
--- a/src/backend/utils/cache/partcache.c
+++ b/src/backend/utils/cache/partcache.c
@@ -263,10 +263,10 @@ RelationBuildPartitionKey(Relation relation)
* Success --- reparent our context and make the relcache point to the
* newly constructed key
*/
- MemoryContextSetParent(partkeycxt, CacheMemoryContext);
if (relation->rd_partinfo == NULL)
relation->rd_partinfo = MemoryContextAllocZero(CacheMemoryContext,
sizeof(RelationPartitionInfo));
+ MemoryContextSetParent(partkeycxt, CacheMemoryContext);
relation->rd_partinfo->partkeycxt = partkeycxt;
relation->rd_partinfo->partkey = key;
}
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index c4b7e01503b..f736543aa78 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -2754,6 +2754,19 @@ RelationRebuildRelation(Relation relation)
relation->rd_partinfo->partkey != NULL);
}
+ /*
+ * Ensure that the destination has storage for partition metadata
+ * before starting the descriptor swap. Nothing in the swap region
+ * may fail, because relation is transiently inconsistent there.
+ */
+ if (!RelationIsIndex(relation) && newrel->rd_partinfo == NULL &&
+ (keep_partkey ||
+ (relation->rd_partinfo != NULL &&
+ (relation->rd_partinfo->pdcxt != NULL ||
+ relation->rd_partinfo->pddcxt != NULL))))
+ newrel->rd_partinfo = MemoryContextAllocZero(CacheMemoryContext,
+ sizeof(RelationPartitionInfo));
+
/*
* Perform swapping of the relcache entry contents. Within this
* process the old entry is momentarily invalid, so there *must* be no
@@ -2819,9 +2832,7 @@ RelationRebuildRelation(Relation relation)
if (keep_partkey)
{
Assert(newrel->rd_partinfo != NULL);
- if (relation->rd_partinfo == NULL)
- relation->rd_partinfo = MemoryContextAllocZero(CacheMemoryContext,
- sizeof(RelationPartitionInfo));
+ Assert(relation->rd_partinfo != NULL);
relation->rd_partinfo->partkey = newrel->rd_partinfo->partkey;
relation->rd_partinfo->partkeycxt = newrel->rd_partinfo->partkeycxt;
newrel->rd_partinfo->partkey = NULL;
@@ -2855,9 +2866,7 @@ RelationRebuildRelation(Relation relation)
* "old" partition descriptor is actually the one hanging off of
* newrel.
*/
- if (relation->rd_partinfo == NULL)
- relation->rd_partinfo = MemoryContextAllocZero(CacheMemoryContext,
- sizeof(RelationPartitionInfo));
+ Assert(relation->rd_partinfo != NULL);
relation->rd_partinfo->partdesc = NULL; /* ensure partdesc is invalid */
relation->rd_partinfo->partdesc_nodetached = NULL;
relation->rd_partinfo->partdesc_nodetached_xmin = InvalidTransactionId;
From: PostgreSQL relcache v3 Linux review
Subject: [PATCH 2/4] Test predicate locking with a rule-bearing table
Exercise TRUNCATE in a serializable transaction after rewrite-rule metadata
has populated the table side of the RelationData union. The unfixed v3
series aborts in CheckTableForSerializableConflictIn().
Depends on patch 1. Applies after David Geier's v3 patches 0001 through
0006, based on 9dbc53591a2de03e0cfdab61a78726f8b2c29eef.
---
diff --git a/src/test/regress/expected/relcache_union.out b/src/test/regress/expected/relcache_union.out
new file mode 100644
index 00000000000..8d77c4cf7d7
--- /dev/null
+++ b/src/test/regress/expected/relcache_union.out
@@ -0,0 +1,14 @@
+-- A rule-bearing table has table-side union members populated.
+CREATE TABLE relcache_union_tab (a integer PRIMARY KEY);
+CREATE RULE relcache_union_rule AS ON INSERT TO relcache_union_tab
+ DO ALSO SELECT 1;
+BEGIN ISOLATION LEVEL SERIALIZABLE;
+INSERT INTO relcache_union_tab VALUES (1);
+ ?column?
+----------
+ 1
+(1 row)
+
+TRUNCATE relcache_union_tab;
+COMMIT;
+DROP TABLE relcache_union_tab;
diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedule
index 75063f87a4a..bb9d90be51e 100644
--- a/src/test/regress/parallel_schedule
+++ b/src/test/regress/parallel_schedule
@@ -143,6 +143,9 @@ test: event_trigger_login
# this test also uses event triggers, so likewise run it by itself
test: fast_default
+# Exercise relcache union discrimination without concurrent DDL.
+test: relcache_union
+
# run tablespace test at the end because it drops the tablespace created during
# setup that other tests may use.
test: tablespace
diff --git a/src/test/regress/sql/relcache_union.sql b/src/test/regress/sql/relcache_union.sql
new file mode 100644
index 00000000000..bdb708fa706
--- /dev/null
+++ b/src/test/regress/sql/relcache_union.sql
@@ -0,0 +1,11 @@
+-- A rule-bearing table has table-side union members populated.
+CREATE TABLE relcache_union_tab (a integer PRIMARY KEY);
+CREATE RULE relcache_union_rule AS ON INSERT TO relcache_union_tab
+ DO ALSO SELECT 1;
+
+BEGIN ISOLATION LEVEL SERIALIZABLE;
+INSERT INTO relcache_union_tab VALUES (1);
+TRUNCATE relcache_union_tab;
+COMMIT;
+
+DROP TABLE relcache_union_tab;
