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

>>> 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)
>>
>> I would prefer to not do that. I like that now we have a single source
>> of truth and adding such a field would be a bunch of code churn.
>
> What specifically would need a bunch of code churn? The paths we're
> dealing with shouldn't need much (if any) changes in that regard. One
> reason to do this is to make it simpler to add another variant to the
> union, if/when we figure out how to split the larger of [table-like,
> index-like] relations further.
>
>> Performance-wise the extra indirection doesn't matter. It's not accessed
>> anywhere in way that it's expensive relative to all the other stuff
>> happening. The pgbench I ran support that claim.
>
> Sure, but for debugging and for interpretability I do think that it
> makes more sense to have an indicator which of the two variants
> should/can be accessed that is stored in the Relation itself. It'll
> also simplify the condition (from two OR-ed options to just one) and
> will make adding future variants (if any) simpler by allowing using
> switch() blocks when we need to handle generic Relations.

Took a stab at it in a new top commit so we can discuss before
integrating it throughout all existing commits.

>>> I think this'll be better if it splits out the Table and Index parts> from RelationBuildDesc.
>>
>> I'm not sure what you're exactly after here. Can you share some more
>> details please?
>
> I meant that the code of RelationBuildDesc would be easier to read and
> understand if we split the code that fills in the shared fields from
> the code that fills in the index variant of the union and from the
> code that fills in the non-index parts of the union, instead of mixing
> those. That doesn't need to be in a different function per se, but
> could also just be a better vertical grouping with a singular if/else
> block.
Done. Now, first comes all shared initialization code, then
if (RelationIsIndex())
with the two legs for index and non-index specific RelationData members.

I'll send out a new patch set once I've addressed the review comments in
your latest mail.

--
David Geier