Subject: Re: Large objects and out-of-memory
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-bugs@lists.postgresql.org
References: <7c57860c-fc05-143e-aa81-f11ca13ec00a@postgrespro.ru>
 <543675.1608575245@sss.pgh.pa.us>
From: Konstantin Knizhnik <k.knizhnik@postgrespro.ru>
Message-ID: <8044c223-4d3a-2cdb-42bf-29940840ce94@postgrespro.ru>
Date: Thu, 24 Dec 2020 17:06:34 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.2.1
MIME-Version: 1.0
In-Reply-To: <543675.1608575245@sss.pgh.pa.us>
Content-Type: multipart/mixed;
 boundary="------------E59924D9528B501CAF332C35"
Content-Language: en-US
Precedence: bulk

This is a multi-part message in MIME format.
--------------E59924D9528B501CAF332C35
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit


On 21.12.2020 21:27, Tom Lane wrote:
> Konstantin Knizhnik <k.knizhnik@postgrespro.ru> writes:
>> The following sequence of command cause backend's memory to exceed 10Gb:
>> INSERT INTO image1 SELECT lo_creat(-1) FROM generate_series(1,10000000);
>> REASSIGN OWNED BY alice TO testlo;
> [ shrug... ]  You're asking to change the ownership of 10000000 objects.
> This is not going to be a cheap operation.  AFAIK it's not going to be
> any more expensive than changing the ownership of 10000000 tables, or
> any other kind of object.
>
> The argument for allowing large objects to have per-object ownership and
> permissions in the first place was that useful scenarios wouldn't have a
> huge number of them (else you'd run out of disk space, if they're actually
> "large"), so we needn't worry too much about the overhead.
>
> We could possibly bound the amount of space used in the inval queue by
> switching to an "invalidate all" approach once we got to an unreasonable
> amount of space.  But this will do nothing for the other costs involved,
> and I'm not really sure it's worth adding complexity for.
>
> 			regards, tom lane

It seems to me that several millions large objects (documents, images, 
... any other blobs) is something less exotic
than several millions tables.
In any case I think that such command REASSIGN should not cause crash of 
the server.

I attached small patch based on your idea: replace individual 
invalidation messages with invalidate-all messages.
If GUC variable invalidate_all_threshold is set to non zero value, then 
there is no memory overflow in this scenario.

I am not sure if the proposed approach to "collapse" invalidation 
message is really good.
Especially I do not like that there is no stored counter of invalidation 
message and I have to traverse all chunk list to calculate it.

Also I noticed small inconsistency in inval.c:

     else if (msg->id == SHAREDINVALSNAPSHOT_ID)
     {
         /* We only care about our own database and shared catalogs */
         if (msg->rm.dbId == InvalidOid)
             InvalidateCatalogSnapshot();
         else if (msg->rm.dbId == MyDatabaseId)
             InvalidateCatalogSnapshot();
     }

Here we are processing snapshot invalidation message but using structure 
for relation map invalidation.
It doesn't cause problems because only dbId field is used and it has the 
same offset
in SharedInvalRelmapMsg and SharedInvalSnapshotMsg structures.
But it should be fixed.

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


--------------E59924D9528B501CAF332C35
Content-Type: text/x-patch;
 name="restricted_inval.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="restricted_inval.patch"

diff --git a/src/backend/utils/cache/inval.c b/src/backend/utils/cache/inval.c
index 628d6f5..eeb6a82 100644
--- a/src/backend/utils/cache/inval.c
+++ b/src/backend/utils/cache/inval.c
@@ -213,6 +213,8 @@ static struct RELCACHECALLBACK
 
 static int	relcache_callback_count = 0;
 
+int invalidate_all_threshold = 0; /* GUC */
+
 /* ----------------------------------------------------------------
  *				Invalidation list support functions
  *
@@ -287,6 +289,81 @@ AppendInvalidationMessageList(InvalidationChunk **destHdr,
 	*destHdr = *srcHdr;
 
 	*srcHdr = NULL;
+
+	if (invalidate_all_threshold > 0)
+	{
+		int n_invalidations = 0;
+
+		/* Count total number of message */
+		for (chunk = *destHdr; chunk != NULL; chunk = chunk->next)
+			n_invalidations += chunk->nitems;
+
+		if (n_invalidations > invalidate_all_threshold)
+		{
+			/* Replace all individual invalidation messages with reset-all ones */
+			InvalidationChunk *tail = *destHdr;
+			int n_tail_msgs = 0;
+			int mask = 0;
+
+			for (chunk = tail; chunk != NULL; chunk = chunk->next)
+			{
+				for (int i = 0; i < chunk->nitems; i++)
+				{
+					switch (chunk->msgs[i].id)
+					{
+					  case SHAREDINVALCATALOG_ID:
+						if (!(mask & (1 << -SHAREDINVALCATALOG_ID)))
+						{
+							tail->msgs[n_tail_msgs].id = SHAREDINVALCATALOG_ID;
+							tail->msgs[n_tail_msgs].cat.dbId = InvalidOid;
+							n_tail_msgs += 1;
+							mask |= (1 << -SHAREDINVALCATALOG_ID);
+						}
+						break;
+					  case SHAREDINVALRELCACHE_ID:
+						if (!(mask & (1 << -SHAREDINVALRELCACHE_ID)))
+						{
+							tail->msgs[n_tail_msgs].id = SHAREDINVALRELCACHE_ID;
+							tail->msgs[n_tail_msgs].rc.dbId = InvalidOid;
+							tail->msgs[n_tail_msgs].rc.relId = InvalidOid;
+							n_tail_msgs += 1;
+							mask |= (1 << -SHAREDINVALRELCACHE_ID);
+						}
+						break;
+					  case SHAREDINVALSNAPSHOT_ID:
+						if (!(mask & (1 << -SHAREDINVALSNAPSHOT_ID)))
+						{
+							tail->msgs[n_tail_msgs].id = SHAREDINVALSNAPSHOT_ID;
+							tail->msgs[n_tail_msgs].sn.dbId = InvalidOid;
+							tail->msgs[n_tail_msgs].sn.relId = InvalidOid;
+							n_tail_msgs += 1;
+							mask |= (1 << -SHAREDINVALSNAPSHOT_ID);
+						}
+						break;
+					  default:
+						tail->msgs[n_tail_msgs++] = chunk->msgs[i];
+					}
+					if (n_tail_msgs == tail->maxitems)
+					{
+						tail->nitems = n_tail_msgs;
+						tail = tail->next;
+						n_tail_msgs = 0;
+					}
+				}
+			}
+			tail->nitems = n_tail_msgs;
+
+			/* remove rest of chain */
+			chunk = tail->next;
+			tail->next = NULL;
+			while (chunk != NULL)
+			{
+				tail = chunk->next;
+				pfree(chunk);
+				chunk = tail;
+			}
+		}
+	}
 }
 
 /*
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 878fcc2..b9bf525 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -90,6 +90,7 @@
 #include "utils/bytea.h"
 #include "utils/float.h"
 #include "utils/guc_tables.h"
+#include "utils/inval.h"
 #include "utils/memutils.h"
 #include "utils/pg_locale.h"
 #include "utils/pg_lsn.h"
@@ -2348,6 +2349,16 @@ static struct config_int ConfigureNamesInt[] =
 		NULL, NULL, NULL
 	},
 
+	{
+		{"invalidate_all_threshold", PGC_SUSET, RESOURCES_MEM,
+			gettext_noop("Limit for maximal number of invalidation messages after reaching which all system caches are invalidateds."),
+			NULL
+		},
+		&invalidate_all_threshold,
+		0, 0, INT_MAX,
+		check_max_stack_depth, assign_max_stack_depth, NULL
+	},
+
 	/*
 	 * We use the hopefully-safely-small value of 100kB as the compiled-in
 	 * default for max_stack_depth.  InitializeGUCOptions will increase it if
diff --git a/src/include/utils/inval.h b/src/include/utils/inval.h
index 463888c..e59d338 100644
--- a/src/include/utils/inval.h
+++ b/src/include/utils/inval.h
@@ -22,6 +22,7 @@
 typedef void (*SyscacheCallbackFunction) (Datum arg, int cacheid, uint32 hashvalue);
 typedef void (*RelcacheCallbackFunction) (Datum arg, Oid relid);
 
+extern int invalidate_all_threshold;
 
 extern void AcceptInvalidationMessages(void);
 

--------------E59924D9528B501CAF332C35--