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

> 0003-0008: These need more commit description than they currently have.

Updated commit messages, including sizeof(RelationData).

> 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.

Done.

> 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.

Done.

> 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.
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.

> 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?

>> +/*
>> + * 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.

Agreed. Also I somehow lost a commit before creating the patch set.
Previously, adding the unions and doing the packing was two commits. I
split the commit again and did the packing such that we reach the
smallest struct size while minimizing the number of code lines moved. I
also removed the comment.

> 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.

Yes. Added an assertion and changed it to use test on rlocator.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.

rd_partdesc got moved into RelationPartitionInfo and is now called
partdesc. I'm assuming you meant RelationData::rd_partinfo?

I went through all places where it's used frequently and stored it in a
local variable.

> 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.

With the latest patch set these two patches no longer reduce the struct
size. Fine by me to exclude them for now. Removed from the latest patch set.

> 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?

My bad. That happened by accident somehow when rebasing on master.
Removed in the new patch set.

I'm sending the new patch set, including the changes to address above
review comments, in my reply to Haibo's review mail.

--
David Geier