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

Hi Matthias!

Thanks for taking another look.

> Though I suggested the macro, I'm very much open to changing the name
> to something better than AllocRelationData().
>
>> @@ -6301,7 +6324,8 @@ load_relcache_init_file(bool shared)
>> rels = repalloc_array(rels, Relation, max_rels);
>> }

I'm generally okay with the name. Maybe AllocRelcacheEntry() would be
better?

>> - rel = rels[num_rels++] = (Relation) palloc(len);
>> + rel = rels[num_rels++] = (Relation)
>> + MemoryContextAlloc(RelationSlabContext, len);
>>
>
> This should probably check that len == sizeof(RelationData), and then
> just use the AllocRelationData() macro used everywhere else.

Changed to use AllocRelationData(). A few lines above there's already

if (len != sizeof(RelationData))
goto read_failed;

Hence, no need for an assertion.

> 0003: I agree with Alvaro's comment; if moving ForeignKeyCacheInfo to
> its own header would also solve the issue then that's preferred.

I'm waiting for Alvaro's and Peter's reply before changing that part.

Any thoughts from your side regarding moving ForeignKeyCacheInfo into
its own header vs building support for anonymous structs/unions in
gen_node_support.pl?

> 0004:
>
>> Table-like relations and indexes use disjoint sets of cached metadata. Put
>> those fields in an anonymous union so each relcache entry pays only for the
>> larger set instead of both sets.
>
> I'd write that as
>
> [...] pays only for the larger of both sets, instead of the sum of both sets.

Done.

>> +++ b/src/backend/storage/lmgr/predicate.c
>> @@ -2558,7 +2558,7 @@ PredicateLockTID(Relation relation, const ItemPointerData *tid, Snapshot snapsho
>> /*
>> * Return if this xact wrote it.
>> */
>> - if (relation->rd_index == NULL)
>> + if (!RelationIsIndex(relation))
>
> You mentioned that the additional lookup is minimal, but I have to
> disagree here. In this case, non-indexes can go into a short path
> that's now one indirection more expensive than before. All the
> changes from rd_index checks to RelationIsIndex() with higher
> indirections may also be a sign that this is a common pattern that's
> become more expensive.

Let's see how that works out. I had started to implement something but
let's better first clarify upfront some details:

Was your idea to simply store a char relkind in struct RelationData? Or
rather a bool is_index, or yet something else?

And then we would change RelationIsIndex() to read that field. Anything
else?

>> - Assert(relation->rd_index == NULL); /* not an index relation */
>> + Assert(!RelationIsIndex(relation));
>
> Let's not drop the comments in the refactoring.

That was intentional. I thought with the new macro the code is as
expressive as the comment. I don't mind putting it back if you think it
improves readability.

>> @@ -2658,9 +2669,9 @@ RelationRebuildRelation(Relation relation)
>
> I think this should be treated similar to
> RelationBuildDesc/RelationDestroyRelation: Have a common path for
> non-union fields, and then a single code block (if+else, or
> switch+cases) for each of the union's variants.

The function cannot be split into just three blocks because there some
dependencies but I streamlined the code flow somewhat without making the
diff impossible to review.

> 0005:
>
> I think maybe we should put RelationPartitionInfo into a Slab
> allocator, too, or somehow figure out how to reduce its size. It's
> currently 8 pointers, plus 5 bytes of scalar fields and 3 bytes of
> alignment. That's just a bit larger than Aset's 64-byte bucket
> boundary, and I'd hate to waste 56 bytes /allocation in highly
> partitioned workloads.

That or we could - FWICS - remove partkeycxt because partkey is always
freed together with partkeycxt and we can recover the context via
GetMemoryChunkContext(partinfo->partkey).

That's less invasive but probably not as clean.

> 0006:
> By splitting enabled/valid -bools away from their pointers, we're
> reducing locality of lookups by a small margin, making accesses on
> just those fields slightly more expensive if they're not found on the
> same cache line. I think the savings can still be worth it, but it's
> something you may want to think about a bit more.

When I tested if moving out the partition members causes regressions, I
also tested if moving away the booleans causes regressions. Back then I
couldn't measure any regression.

If we ever find a regression related to that we could always switch to
the sentinel pointer pattern. I know that you don't like that too much
but it would give us both.

> Nit:
>> + bool pgstat_enabled; /* should relation stats be counted */
> This is a pre-existing nit, but I'm not happy with the inconsistent
> name of this field (and its sibling pgstat_info). All other fields are
> rd_-prefixed, except these pgstat_-prefixed fields.

+1 but let's do that in a separate thread.

> 0007:
>> +++ b/src/backend/access/nbtree/nbtutils.c
>> +#include "storage/lmgr.h"
>
>> +++ b/src/include/utils/rel.h
>> +#include "miscadmin.h"
>
> Why did we need add includes here, when both already should have
> rel.h's full definitions included, and the changes are relevant only
> for things that touch(ed) RelationData directly?

* The lmgr.h include is unnecessary. Removed.
* The miscadmin.h include is necessary for MyDatabaseId.

I've also created a commitfest entry, see [1].

--
David Geier

[1] https://commitfest.postgresql.org/patch/7483/