On Fri, Oct 16, 2026 at 6:02 AM David Geier <[email protected]> wrote:
>
> Hi Haibo,
>
> Thanks for your review and kudos for providing test cases and fixes.
> Much appreciated.
>
> > 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've removed 0007 and 0008 from the new patch set.
>
> > 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.
>
> Fixed the problem using your patch. I don't think we should include the
> test cases as it's very specific. Opinions?
>
> > 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"
>
> Good catch!
>
> > 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.
>
> Fixed the problem using your patch.
>
> > 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.
>
> Fixed as well. Additionally moved into the else branch of the check
>
> if (!RelationIsIndex(rel))
>
> so that all code that resets lazily built fields is in one place.
>
> > 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.
>
> With the latest patch set (excluding 0007 and 0008), we go from 496
> bytes on master to 248 bytes.
>
> > 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.
>
> Removed in latest patch set.
>
>
> Attached is v4 of the patch set.
>
> Beyond that:
>
> 1. Bumped RELCACHE_INIT_FILEMAGIC in any patch that makes structural
> changes to struct RelationData, so that PostgreSQL rebuilds old init
> files if necessary.
>
> 2. Moved many more members that are relation-only to the corresponding
> union leg.
>
> 3. Moved members related to partitioning out of the relation-only table
> leg because indexes can be partitioned as well.
>
> --
> David Geier
Hi David,
I took a closer look at the relation-kind-specific field overlay
introduced by patch 0004, particularly the relcache accessors and
initialization paths affected by the new union.
I found one API compatibility regression and two minor issues worth
considering for v5.
1. RelationGetIndexExpressions(), RelationGetDummyIndexExpressions(),
and RelationGetIndexPredicate()
RelationGetIndexExpressions() and RelationGetIndexPredicate()
explicitly document that they return NIL for non-index relations.
The new union changes this behavior because several index-specific
fields overlap with table-specific cache fields:
rd_indexprs / rd_pkattr
rd_indpred / rd_idattr
rd_indextuple / rd_rulescxt
With the table caches populated, these functions can return
incorrectly typed pointers or encounter errors. In some builds,
the invalid access can also cause a backend crash.
I reproduced this using a small C test extension with real
Relation objects:
- Master: NIL for all tested non-index relations.
- Original v4: incorrect non-NIL results, ERRORs, or crashes.
- Fixed v4: NIL for all tested non-index relations.
RelationGetDummyIndexExpressions() is also affected by the
rd_indextuple overlap. Although its comment does not explicitly
promise NIL for non-index relations, that was its previous behavior.
The minimal fix is to add the following check at the beginning
of all three functions:
if (!RelationIsIndex(relation))
return NIL;
I also checked the core callers and found no path that passes
a non-index Relation to these functions. Therefore, this appears
to be an API compatibility regression for backend C callers,
rather than a bug reachable through ordinary SQL.
2. RelationCacheInitializePhase3()
The condition checks rd_tableam before checking the relation kind.
For indexes, rd_tableam overlaps with rd_indcollation.
Reversing the condition would avoid reading the inactive union
member. I found no observable correctness issue here.
3. RelationBuildDesc()
The unconditional initialization of rd_fkeylist, rd_fkeyvalid,
and rd_toastchunkidtype also writes to storage used by
index-specific fields.
These writes currently appear harmless because the allocation
is zero-initialized and the index fields are initialized later.
Guarding these assignments with !RelationIsIndex() would make
the union usage clearer and less fragile.
I validated the proposed changes on Linux/GCC and macOS/Clang.
The targeted tests passed with the fixes, as did the regression
and isolation suites. The Linux fixed-v4 build also passed
check-world with TAP tests enabled.
Overall, the series looks good to me with the API regression
addressed. The other two changes are minor cleanups.
Thanks,
Haibo
