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

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