pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedLarge objects and out-of-memory
5+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2020-12-28 643428c54b 30803bd1cd 190f0af0d1 6819380dd2 1c7ca61789 7bf20c1b8a 8fcc2fb980 Fix inconsistent code with shared invalidations of snapshots
* Large objects and out-of-memory
@ 2020-12-21 16:33 Konstantin Knizhnik <k.knizhnik@postgrespro.ru>
0 siblings, 1 reply; 5+ messages in thread
From: Konstantin Knizhnik @ 2020-12-21 16:33 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org
Hi,
The following sequence of command cause backend's memory to exceed 10Gb:
CREATE DATABASE testmem;
CREATE ROLE alice WITH SUPERUSER LOGIN;
CREATE ROLE testlo WITH SUPERUSER LOGIN;
\c testmem # Подключаемся к тестовой базе
\c - alice
CREATE TABLE image1 (raster oid);
INSERT INTO image1 SELECT lo_creat(-1) FROM generate_series(1,10000000);
REASSIGN OWNED BY alice TO testlo;
This memory is occupied in top memory context by invalidation messages:
#0 AllocSetAlloc (context=0x19153a0, size=528) at aset.c:919
#1 0x0000000000b451d5 in MemoryContextAlloc (context=0x19153a0,
size=528) at mcxt.c:809
#2 0x0000000000ae80c6 in AddInvalidationMessage (listHdr=0x19156d8,
msg=0x7ffdca880600) at inval.c:241
#3 0x0000000000ae8473 in AddSnapshotInvalidationMessage (hdr=0x19156d0,
dbId=0, relId=1214) at inval.c:437
#4 0x0000000000ae871c in RegisterSnapshotInvalidation (dbId=0,
relId=1214) at inval.c:547
#5 0x0000000000ae915f in CacheInvalidateHeapTuple
(relation=0x7f16afc35cb0, tuple=0x7ffdca8807b0, newtuple=0x198dba8) at
inval.c:1162
#6 0x00000000004f2d41 in heap_update (relation=0x7f16afc35cb0,
otid=0x198dbac, newtup=0x198dba8, cid=6, crosscheck=0x0, wait=true,
tmfd=0x7ffdca880830,
lockmode=0x7ffdca880828) at heapam.c:3729
#7 0x00000000004f3913 in simple_heap_update (relation=0x7f16afc35cb0,
otid=0x198dbac, tup=0x198dba8) at heapam.c:3891
#8 0x00000000005c281d in CatalogTupleUpdate (heapRel=0x7f16afc35cb0,
otid=0x198dbac, tup=0x198dba8) at indexing.c:309
#9 0x00000000005e7713 in shdepChangeDep (sdepRel=0x7f16afc35cb0,
classid=2613, objid=11950311, objsubid=0, refclassid=1260, refobjid=32848,
deptype=SHARED_DEPENDENCY_OWNER) at pg_shdepend.c:269
#10 0x00000000005e784c in changeDependencyOnOwner (classId=2613,
objectId=11950311, newOwnerId=32848) at pg_shdepend.c:316
#11 0x0000000000669844 in AlterObjectOwner_internal (rel=0x7f16afc34c90,
objectId=11950311, new_ownerId=32848) at alter.c:1050
#12 0x00000000005e9c8e in shdepReassignOwned (roleids=0x19aa138,
newrole=32848) at pg_shdepend.c:1587
#13 0x0000000000718aa2 in ReassignOwnedObjects (stmt=0x18f1830) at
user.c:1425
#14 0x000000000097bf7d in standard_ProcessUtility (pstmt=0x18f1b80,
queryString=0x18f0cd0 "REASSIGN OWNED BY alice TO testlo;",
context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0,
dest=0x18f1c70, qc=0x7ffdca881180) at utility.c:890
#15 0x000000000097b508 in ProcessUtility (pstmt=0x18f1b80,
queryString=0x18f0cd0 "REASSIGN OWNED BY alice TO testlo;",
context=PROCESS_UTILITY_TOPLEVEL,
As far as I understand invalidation messages are kept until the end of
transaction:
/*
* CommandEndInvalidationMessages
* Process queued-up invalidation messages at end of one command
* in a transaction.
*
* Here, we send no messages to the shared queue, since we don't know
yet if
* we will commit. We do need to locally process the CurrentCmdInvalidMsgs
* list, so as to flush our caches of any entries we have outdated in the
* current command. We then move the current-cmd list over to become part
* of the prior-cmds list.
*
* Note:
* This should be called during CommandCounterIncrement(),
* after we have advanced the command ID.
*/
void
CommandEndInvalidationMessages(void)
{
AppendInvalidationMessages(&transInvalInfo->PriorCmdInvalidMsgs,
&transInvalInfo->CurrentCmdInvalidMsgs);
}
So in PriorCmdInvalidMsgs we have 10 millions invalidation message chunks.
Such memory blow happens in this scenario twice: when large objects are
created and when owner is reassigned.
It can not be considered as memory leak, but I think that it is a real
problem which has to be fixed.
I am not so familiar with invalidation message processing, I will be
pleae if somebody can suggest solution of the problem.
Thanks in advance,
--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Large objects and out-of-memory
@ 2020-12-21 18:27 Tom Lane <tgl@sss.pgh.pa.us>
parent: Konstantin Knizhnik <k.knizhnik@postgrespro.ru>
0 siblings, 1 reply; 5+ messages in thread
From: Tom Lane @ 2020-12-21 18:27 UTC (permalink / raw)
To: Konstantin Knizhnik <k.knizhnik@postgrespro.ru>; +Cc: pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Large objects and out-of-memory
@ 2020-12-24 14:06 Konstantin Knizhnik <k.knizhnik@postgrespro.ru>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 5+ messages in thread
From: Konstantin Knizhnik @ 2020-12-24 14:06 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-bugs@lists.postgresql.org
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
Attachments:
restricted_inval.patch (text/x-patch, 3.9K) download
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);
* Re: Large objects and out-of-memory
@ 2020-12-25 01:51 Michael Paquier <michael@paquier.xyz>
parent: Konstantin Knizhnik <k.knizhnik@postgrespro.ru>
0 siblings, 1 reply; 5+ messages in thread
From: Michael Paquier @ 2020-12-25 01:51 UTC (permalink / raw)
To: Konstantin Knizhnik <k.knizhnik@postgrespro.ru>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-bugs@lists.postgresql.org
On Thu, Dec 24, 2020 at 05:06:34PM +0300, Konstantin Knizhnik wrote:
> 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.
Good catch. It is very easy to miss that this is not "rm", but "sn".
I have checked the rest of this area and did not notice similar
errors. I'll backpatch the attached if there are no objections. This
may look cosmetic, but this would cause unnecessary conflicts if this
area gets touched and it would be hard to miss. This comes from
568d413. There may be a point in having a single call to
InvalidateCatalogSnapshot() as well..
--
Michael
Attachments:
inval-inconsistent.patch (text/x-diff, 620B) download
diff --git a/src/backend/utils/cache/inval.c b/src/backend/utils/cache/inval.c
index 628d6f5d0c..e7279d06a3 100644
--- a/src/backend/utils/cache/inval.c
+++ b/src/backend/utils/cache/inval.c
@@ -622,9 +622,9 @@ LocalExecuteInvalidationMessage(SharedInvalidationMessage *msg)
else if (msg->id == SHAREDINVALSNAPSHOT_ID)
{
/* We only care about our own database and shared catalogs */
- if (msg->rm.dbId == InvalidOid)
+ if (msg->sn.dbId == InvalidOid)
InvalidateCatalogSnapshot();
- else if (msg->rm.dbId == MyDatabaseId)
+ else if (msg->sn.dbId == MyDatabaseId)
InvalidateCatalogSnapshot();
}
else
* Re: Large objects and out-of-memory
@ 2020-12-28 13:22 Michael Paquier <michael@paquier.xyz>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 5+ messages in thread
From: Michael Paquier @ 2020-12-28 13:22 UTC (permalink / raw)
To: Konstantin Knizhnik <k.knizhnik@postgrespro.ru>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-bugs@lists.postgresql.org
On Fri, Dec 25, 2020 at 10:51:00AM +0900, Michael Paquier wrote:
> Good catch. It is very easy to miss that this is not "rm", but "sn".
> I have checked the rest of this area and did not notice similar
> errors. I'll backpatch the attached if there are no objections.
Applied this one with 643428c & co.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2020-12-28 13:22 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-12-21 16:33 Large objects and out-of-memory Konstantin Knizhnik <k.knizhnik@postgrespro.ru>
2020-12-21 18:27 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-12-24 14:06 ` Konstantin Knizhnik <k.knizhnik@postgrespro.ru>
2020-12-25 01:51 ` Michael Paquier <michael@paquier.xyz>
2020-12-28 13:22 ` Michael Paquier <michael@paquier.xyz>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox