Re: Reducing relcache memory usage: deduping index shapes - Mailing list pgsql-hackers

>Attached is v2 of the patchset. Changelist below:
>
>0001/0002: Unchanged.
>
>0003 (Deduplication):
>* New memory context "Relation shape cache" to hold all shape-related
>allocations and contexts,
>    This is a child context under CacheMemoryContext
>* Memory context per "relation shape" in the RelShapeHash
>    This allows faster freeing of all associated data
>* Optimized default shape entry data allocations per shape
>    The minimum is now down to 2 allocations, from >3. This is
>primarily useful once Proxy contexts are used;  Key data is still
>bulk-allocated.
>* Some varlena macro-related fixes.
>    Some SIZE/SIZE_EXHDR confusion and related issues, identified by
>the sanitizer CF builds.
>
>0004 (Proxy context):
>* Consistency checks have been adjusted, and sentinel checks have been
>introduced.
>* Code has been updated with aset as template for naming and flow
>  This should clean up David's comments.
>
>Name change from Proxy to anything else gets a 0-vote from me: I'd
>like to avoid the churn, but if people have strong feelings about it
>I'll go through the motions.
>
>
>0005 (Apply proxy):
>* Added Proxy to the new per-"index shape" contexts.
>
>
>Question for the crowd:  Most memory contexts often get a text
>identifier which describes their contents in more detail when we have
>many of the same name.  "index shape" contexts don't have a simple
>natural identifier.  Whilst they do have the shape key, formatting
>that into a name would be a bit of effort (and quite a bit of effort
>if we want to capture the whole key), and we'd spend more bytes per
>index shape.  Do we want/need this identifier even with the increase
>in memory usage?
>
>
>Kind regards,
>
>Matthias van de Meent
>Databricks (https://www.databricks.com)


Hi,
Thanks for your patches.
I have some review comments regarding the code below:
v2-0003-Deduplicate-some-index-attributes-in-the-relcache.patch
@@ -2461,6 +2656,28 @@ RelationDestroyRelation(Relation relation, bool remember_tupdesc)
        /* break mutual link with stats entry */
        pgstat_unlink_relation(relation);

+       if (relation->rd_isvalid && relation->rd_indam)
+       {
+               Datum           indclassDatum;
+               bool            isnull;
+               oidvector  *indclass;
+
+               Assert(relation->rd_indextuple != NULL);
+
+               indclassDatum = fastgetattr(relation->rd_indextuple,
+                                                                  Anum_pg_index_indclass,
+                                                                  GetPgIndexDescriptor(),
+                                                                  &isnull);
+               Assert(!isnull);
+
+               indclass = (oidvector *) DatumGetPointer(indclassDatum);
+
+               IndexSupportDeregister(indclass,
+                                                          IndexRelationGetNumberOfKeyAttributes(relation),
+                                                          relation->rd_indam->amsupport,
+                                                          relation->rd_opcoptions);
+       }
relcache.c:
static void
RelationClearRelation(Relation relation)
{
	Assert(RelationHasReferenceCountZero(relation));
	Assert(!relation->rd_isnailed);

	/*
	 * Relations created in the same transaction must never be removed, see
	 * RelationFlushRelation.
	 */
	Assert(relation->rd_createSubid == InvalidSubTransactionId);
	Assert(relation->rd_firstRelfilelocatorSubid == InvalidSubTransactionId);
	Assert(relation->rd_droppedSubid == InvalidSubTransactionId);

	/* first mark it as invalid */
	RelationInvalidateRelation(relation);

	/* Remove it from the hash table */
	RelationCacheDelete(relation);

	/* And release storage */
	RelationDestroyRelation(relation, false);
}

The IndexSupportDeregister() cleanup inside RelationDestroyRelation() cannot be executed, 
since relation->rd_isvalid = false has already been set in RelationInvalidateRelation().
Entries in RelShapeCache cannot be removed.
Is this inconsistent with your design?
Run with the SQL below:
CREATE TABLE m (a int);
INSERT INTO m SELECT g FROM generate_series(1,500) g;
CREATE INDEX mi ON m USING brin (a int4_bloom_ops (false_positive_rate = 0.00100000)); DROP INDEX mi;
CREATE INDEX mi ON m USING brin (a int4_bloom_ops (false_positive_rate = 0.00100100)); DROP INDEX mi;
CREATE INDEX mi ON m USING brin (a int4_bloom_ops (false_positive_rate = 0.00100200)); DROP INDEX mi;
CREATE INDEX mi ON m USING brin (a int4_bloom_ops (false_positive_rate = 0.00100300)); DROP INDEX mi;
...
When constructed in this way, the number of potential RelShapeCache entries may grow far larger than expected.




Best regards,
Yilin Zhang