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

Hi Matthias,

Thanks for the review.

> [0001-0003]
> I Haven't looked at these in detail. Yes, they provide the largest
> savings, but that also makes them most complicated to review in
> detail. I'll stave that off for now.

While the diff is relatively big, the changes are quite mechanical. The
biggest challenge is knowing that all code has been adapted. Apart from
grepping, I relied on the tests for that.

>> - 0004: Removes the rd_lockinfo member. RelationGetLockRelId() now
>> computes it when needed. This also removes RelationInitLockInfo() and
>> the initialization work associated with it.
>> => sizeof(RelationData) == 304 bytes
>
> Can we really rely on rd_rel always being valid when we need the
> LockInfo? I'm OK with avoiding duplicating
> rd_id/rd_lockinfo.lockRelId.relId, but I'm not sure dbId can always be
> derived with rd_rel->relisshared whenever we need it.

The information is derived exactly the same way, just deferred to when
it's needed. The old RelationInitLockInfo() did:

if (relation->rd_rel->relisshared)
relation->rd_lockInfo.lockRelId.dbId = InvalidOid;
else
relation->rd_lockInfo.lockRelId.dbId = MyDatabaseId;

Beyond that, every Relation that can reach RelationGetLockRelId() has
rd_rel populated. There's no code that constructs a Relation with rd_rel
== NULL and then locks it.

There's also one subtle improvement: the deferring version is more
correct than before in the load_relcache_init_file() case. The old code
had to recompute lockRelId after loading pg_internal.init because the
file might have been copied from another database by CREATE DATABASE —
baking in the wrong dbId. Computing MyDatabaseId on the fly makes that
recomputation unnecessary.

>> - 0005: Replaces the embedded partition key, descriptors, partition
>> qual, validity flag, and memory contexts with one lazily allocated
>> RelationPartitionInfo pointer.
>> => sizeof(RelationData) == 264 bytes
>
> I haven't worked on partitioning, so I'm not fully confident that this
> has sufficiently low additional overhead to be worth applying.

I'll run a partitioned pgbench and report if I could measure any
regressions.

>> - 0006: Removes rd_fkeyvalid by using RELCACHE_FKEYLIST_NOT_LOADED as
>> the initial state of rd_fkeylist. NIL continues to mean that the list
>> was computed and no foreign keys were found. This patch is not strictly
>> needed because it currently doesn't further reduce the size. The same we
>> could with RelationPartitionInfo::partcheckvalid.
>> => sizeof(RelationData) == 264 bytes
>
> I'm not a fan of this change. Sentinel values *can* have their place,
> but I really don't like non-NULL values being used to signal "invalid"
> states, and seeing that it doesn't actually save any bytes I'd prefer
> to not add this complication.

The pointer itself has no invalid state. NULL is also a valid state.
This is why the extra flag is needed.

I'm in slight favor of keeping it because "is valid" booleans always
have the tendency to cause padding and I'm hopeful that we can further
strip down the struct so that the change is actually yielding a real saving.

I'm keeping the commit in the patch set for now, waiting for other
reviewers to chime in.

>> - 0008: Adds a static assertion that sizeof(RelationData) <= 256.
>> => sizeof(RelationData) == 256 bytes
>
> I'm not a fan of this, because we generally don't add assertions on
> struct sizes unless it's critically important a struct remains the
> asserted size (or, in this case, doesn't become larger than that).
> In this case, I don't think it's critically important that the struct
> fits in the 256-byte aset bucket, given that relcache memory usage has
> never been capped, and a slab context would similarly do the trick for
> avoiding memory usage cliffs when the size of the struct is increased.
>
>> Note that the necessity to arrive at 256 bytes stems from ASET's
>> allocation granularity being powers of two. To profit from further size
>> reductions of RelationData, we would need to use a SLAB memory context
>> because it's unlikely that we'll get to 128 bytes.

I've removed the patch because of changing to a slab memory context. See
comment below.

> Let's start with palloc-ing all RelationData into Slab contexts (like
> attached, tagged with .nocfbot to avoid your patch's CI). This allows
> us to start saving bytes immediately, and immediately improves memory
> usage for every reduction in size so that the merits of each later
> patch can be evaluated separately. This will be very useful if we
> can't agree on some of the changes that are necessary to reduce the
> size down to the next smaller aset size bucket.

Attached is patch 0002 that allocates all RelationData objects inside a
slab memory context. It's passing all tests together with all the other
patches.

I've also attached patch 0001 that removes two unnecessary checks for if
CacheMemoryContext is already initialized. This is no longer necessary
since #1aebc3618a0, #46287bd6602 and #d4d1885e42e. I wanted to get rid
of these checks so that I don't have to create the slab context in these
places as well.

Attached is the latest patch set rebased also on latest master. While
looking more closely at RelationData, I've realized that we can also
move rd_indexlist, rd_statlist, rd_pkindex, rd_toastoid and
rd_toastchunkidtype to the table union leg. As the index union leg is
currently bigger, that should yield some more savings. I'll give this a
try the next days.

--
David Geier