Thanks for taking another look. > 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;
Changed all three functions as suggested by you. > 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. Changed. > 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. Changed. I'll soon send the updated patch set in reply to Matthias mail. -- David Geier
