pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedmem context is not reset between extended stats
5+ messages / 2 participants
[nested] [flat]
* mem context is not reset between extended stats
@ 2021-09-15 20:09 Justin Pryzby <pryzby@telsasoft.com>
2021-09-21 00:15 ` Re: mem context is not reset between extended stats Tomas Vondra <tomas.vondra@enterprisedb.com>
0 siblings, 1 reply; 5+ messages in thread
From: Justin Pryzby @ 2021-09-15 20:09 UTC (permalink / raw)
To: Tomas Vondra <tomas.vondra@enterprisedb.com>; +Cc: pgsql-hackers
Memory allocation appeared be O(1) WRT the number of statistics objects, which
was not expected to me. This is true in v13 (and probably back to v10).
It seems to work fine to reset the memory context within the loop, so long as
the statslist is allocated in the parent context.
|DROP TABLE t; CREATE TABLE t AS SELECT i, i+1 AS a, i+2 AS b, i+3 AS c, i+4 AS d, i+5 AS e FROM generate_series(1,99999)i;
|SELECT format('CREATE STATISTICS sta%s (ndistinct) ON a,(1+b),(2+c),(3+d),(4+e) FROM t', a) FROM generate_series(1,9)a\gexec
|SET log_statement_stats=on; SET client_min_messages=debug; ANALYZE t;
|=> 369432 kB max resident size
|SELECT format('CREATE STATISTICS sta%s (ndistinct) ON a,b,c,d,e FROM t', a) FROM generate_series(1,33)a\gexec
|SET log_statement_stats=on; SET client_min_messages=debug; ANALYZE t;
|=> 1284368 kB max resident size
Attachments:
[text/x-diff] 0001-stx-do-not-leak-memory-for-each-stats-obj.patch (1.6K, ../../20210915200928.GP831@telsasoft.com/2-0001-stx-do-not-leak-memory-for-each-stats-obj.patch)
download | inline diff:
From 6ae4d059f2ed8baf2af92ec0847458055147383c Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Wed, 15 Sep 2021 14:38:49 -0500
Subject: [PATCH] stx: do not leak memory for each stats obj
---
src/backend/statistics/extended_stats.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/src/backend/statistics/extended_stats.c b/src/backend/statistics/extended_stats.c
index 63549757ec..d9387ceeb6 100644
--- a/src/backend/statistics/extended_stats.c
+++ b/src/backend/statistics/extended_stats.c
@@ -136,14 +136,14 @@ BuildRelationExtStatistics(Relation onerel, double totalrows,
if (!natts)
return;
+ pg_stext = table_open(StatisticExtRelationId, RowExclusiveLock);
+ statslist = fetch_statentries_for_relation(pg_stext, RelationGetRelid(onerel));
+
cxt = AllocSetContextCreate(CurrentMemoryContext,
"BuildRelationExtStatistics",
ALLOCSET_DEFAULT_SIZES);
oldcxt = MemoryContextSwitchTo(cxt);
- pg_stext = table_open(StatisticExtRelationId, RowExclusiveLock);
- statslist = fetch_statentries_for_relation(pg_stext, RelationGetRelid(onerel));
-
/* report this phase */
if (statslist != NIL)
{
@@ -245,14 +245,12 @@ BuildRelationExtStatistics(Relation onerel, double totalrows,
pgstat_progress_update_param(PROGRESS_ANALYZE_EXT_STATS_COMPUTED,
++ext_cnt);
- /* free the build data (allocated as a single chunk) */
- pfree(data);
+ MemoryContextReset(cxt);
}
- table_close(pg_stext, RowExclusiveLock);
-
MemoryContextSwitchTo(oldcxt);
MemoryContextDelete(cxt);
+ table_close(pg_stext, RowExclusiveLock);
}
/*
--
2.17.0
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: mem context is not reset between extended stats
2021-09-15 20:09 mem context is not reset between extended stats Justin Pryzby <pryzby@telsasoft.com>
@ 2021-09-21 00:15 ` Tomas Vondra <tomas.vondra@enterprisedb.com>
2021-09-21 01:37 ` Re: mem context is not reset between extended stats Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 1 reply; 5+ messages in thread
From: Tomas Vondra @ 2021-09-21 00:15 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: pgsql-hackers
On 9/15/21 10:09 PM, Justin Pryzby wrote:
> Memory allocation appeared be O(1) WRT the number of statistics objects, which
> was not expected to me. This is true in v13 (and probably back to v10).
>
> It seems to work fine to reset the memory context within the loop, so long as
> the statslist is allocated in the parent context.
>
Yeah, and I agree this fix seems reasonable. Thanks for looking!
In principle we don't expect too many extended statistics on a single
table, but building a single statistics may use quite a bit of memory,
so it makes sense to release it early ...
But while playing with this a bit more, I discovered a much worse issue.
Consider this:
create table t (a text, b text, c text, d text,
e text, f text, g text, h text);
insert into t select x, x, x, x, x, x, x, x from (
select md5(mod(i,100)::text) as x
from generate_series(1,30000) s(i)) foo;
create statistics s (dependencies) on a, b, c, d, e, f, g, h from t;
analyze t;
This ends up eating insane amounts of memory - on my laptop it eats
~2.5GB and then crashes with OOM. This happens because each call to
dependency_degree does build_sorted_items, which detoasts the values.
And resetting the context can't fix that, because this happens while
building a single statistics object.
IMHO the right fix is to run dependency_degree in a separate context,
and reset it after each dependency. This releases the detoasted values,
which are otherwise hard to deal with.
This does not mean we should not do what your patch does too. That does
address various other "leaks" (for example MCV calls build_sorted_items
too, but only once so it does not have this same issue).
These issues exist pretty much since PG10, which is where extended stats
were introduced, so we'll have to backpatch it. But there's no rush and
I don't want to interfere with rc1 at the moment.
Attached are two patches - 0001 is your patch (seems fine, but I looked
only very briefly) and 0002 is the context reset I proposed.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
[text/x-patch] 0001-reset-context.patch (1.6K, ../../cd3af9ee-fc08-c9b0-e7c9-d3eb6fb74df6@enterprisedb.com/2-0001-reset-context.patch)
download | inline diff:
From 204f4602b218ec13ac1e3fa501a7f94adc8a4ea1 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas.vondra@postgresql.org>
Date: Tue, 21 Sep 2021 01:14:11 +0200
Subject: [PATCH 1/3] reset context
---
src/backend/statistics/extended_stats.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/src/backend/statistics/extended_stats.c b/src/backend/statistics/extended_stats.c
index 2e55913bc8..5c18da02c7 100644
--- a/src/backend/statistics/extended_stats.c
+++ b/src/backend/statistics/extended_stats.c
@@ -125,14 +125,14 @@ BuildRelationExtStatistics(Relation onerel, double totalrows,
if (!natts)
return;
+ pg_stext = table_open(StatisticExtRelationId, RowExclusiveLock);
+ statslist = fetch_statentries_for_relation(pg_stext, RelationGetRelid(onerel));
+
cxt = AllocSetContextCreate(CurrentMemoryContext,
"BuildRelationExtStatistics",
ALLOCSET_DEFAULT_SIZES);
oldcxt = MemoryContextSwitchTo(cxt);
- pg_stext = table_open(StatisticExtRelationId, RowExclusiveLock);
- statslist = fetch_statentries_for_relation(pg_stext, RelationGetRelid(onerel));
-
/* report this phase */
if (statslist != NIL)
{
@@ -234,14 +234,12 @@ BuildRelationExtStatistics(Relation onerel, double totalrows,
pgstat_progress_update_param(PROGRESS_ANALYZE_EXT_STATS_COMPUTED,
++ext_cnt);
- /* free the build data (allocated as a single chunk) */
- pfree(data);
+ MemoryContextReset(cxt);
}
- table_close(pg_stext, RowExclusiveLock);
-
MemoryContextSwitchTo(oldcxt);
MemoryContextDelete(cxt);
+ table_close(pg_stext, RowExclusiveLock);
}
/*
--
2.31.1
[text/x-patch] 0002-build-dependencies-in-context.patch (1.9K, ../../cd3af9ee-fc08-c9b0-e7c9-d3eb6fb74df6@enterprisedb.com/3-0002-build-dependencies-in-context.patch)
download | inline diff:
From cd0a3398fd6cb586d373bba3d988e564ee7e8c5a Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas.vondra@postgresql.org>
Date: Tue, 21 Sep 2021 01:13:11 +0200
Subject: [PATCH 2/3] build dependencies in context
---
src/backend/statistics/dependencies.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/src/backend/statistics/dependencies.c b/src/backend/statistics/dependencies.c
index d703e9b9ba..4efba06fd6 100644
--- a/src/backend/statistics/dependencies.c
+++ b/src/backend/statistics/dependencies.c
@@ -30,6 +30,7 @@
#include "utils/fmgroids.h"
#include "utils/fmgrprotos.h"
#include "utils/lsyscache.h"
+#include "utils/memutils.h"
#include "utils/selfuncs.h"
#include "utils/syscache.h"
#include "utils/typcache.h"
@@ -325,12 +326,6 @@ dependency_degree(StatsBuildData *data, int k, AttrNumber *dependency)
group_size++;
}
- if (items)
- pfree(items);
-
- pfree(mss);
- pfree(attnums_dep);
-
/* Compute the 'degree of validity' as (supporting/total). */
return (n_supporting_rows * 1.0 / data->numrows);
}
@@ -359,9 +354,14 @@ statext_dependencies_build(StatsBuildData *data)
/* result */
MVDependencies *dependencies = NULL;
+ MemoryContext cxt;
Assert(data->nattnums >= 2);
+ cxt = AllocSetContextCreate(CurrentMemoryContext,
+ "sorted items cxt",
+ ALLOCSET_DEFAULT_SIZES);
+
/*
* We'll try build functional dependencies starting from the smallest ones
* covering just 2 columns, to the largest ones, covering all columns
@@ -380,10 +380,16 @@ statext_dependencies_build(StatsBuildData *data)
{
double degree;
MVDependency *d;
+ MemoryContext oldcxt;
+
+ oldcxt = MemoryContextSwitchTo(cxt);
/* compute how valid the dependency seems */
degree = dependency_degree(data, k, dependency);
+ MemoryContextSwitchTo(oldcxt);
+ MemoryContextReset(cxt);
+
/*
* if the dependency seems entirely invalid, don't store it
*/
--
2.31.1
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: mem context is not reset between extended stats
2021-09-15 20:09 mem context is not reset between extended stats Justin Pryzby <pryzby@telsasoft.com>
2021-09-21 00:15 ` Re: mem context is not reset between extended stats Tomas Vondra <tomas.vondra@enterprisedb.com>
@ 2021-09-21 01:37 ` Justin Pryzby <pryzby@telsasoft.com>
2021-09-21 11:28 ` Re: mem context is not reset between extended stats Tomas Vondra <tomas.vondra@enterprisedb.com>
0 siblings, 1 reply; 5+ messages in thread
From: Justin Pryzby @ 2021-09-21 01:37 UTC (permalink / raw)
To: Tomas Vondra <tomas.vondra@enterprisedb.com>; +Cc: pgsql-hackers
On Tue, Sep 21, 2021 at 02:15:45AM +0200, Tomas Vondra wrote:
> On 9/15/21 10:09 PM, Justin Pryzby wrote:
> > Memory allocation appeared be O(1) WRT the number of statistics objects, which
> > was not expected to me. This is true in v13 (and probably back to v10).
Of course I meant to say that it's O(N) and not O(1) :)
> In principle we don't expect too many extended statistics on a single table,
Yes, but note that expression statistics make it more reasonable to have
multiple extended stats objects. I noticed this while testing a patch to build
(I think) 7 stats objects on each of our current month's partitions.
autovacuum was repeatedly killed on this vm after using using 2+GB RAM,
probably in part because there were multiple autovacuum workers handling the
most recent batch of inserted tables.
First, I tried to determine what specifically was leaking so badly, and
eventually converged to this patch. Maybe there's additional subcontexts which
would be useful, but the minimum is to reset between objects.
> These issues exist pretty much since PG10, which is where extended stats
> were introduced, so we'll have to backpatch it. But there's no rush and I
> don't want to interfere with rc1 at the moment.
Ack that. It'd be *nice* if if the fix were included in v14.0, but I don't
know the rules about what can change after rc1.
> Attached are two patches - 0001 is your patch (seems fine, but I looked only
> very briefly) and 0002 is the context reset I proposed.
I noticed there seems to be a 3rd patch available, which might either be junk
for testing or a cool new feature I'll hear about later ;)
> From 204f4602b218ec13ac1e3fa501a7f94adc8a4ea1 Mon Sep 17 00:00:00 2001
> From: Tomas Vondra <tomas.vondra@postgresql.org>
> Date: Tue, 21 Sep 2021 01:14:11 +0200
> Subject: [PATCH 1/3] reset context
cheers,
--
Justin
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: mem context is not reset between extended stats
2021-09-15 20:09 mem context is not reset between extended stats Justin Pryzby <pryzby@telsasoft.com>
2021-09-21 00:15 ` Re: mem context is not reset between extended stats Tomas Vondra <tomas.vondra@enterprisedb.com>
2021-09-21 01:37 ` Re: mem context is not reset between extended stats Justin Pryzby <pryzby@telsasoft.com>
@ 2021-09-21 11:28 ` Tomas Vondra <tomas.vondra@enterprisedb.com>
2021-09-23 17:04 ` Re: mem context is not reset between extended stats Tomas Vondra <tomas.vondra@enterprisedb.com>
0 siblings, 1 reply; 5+ messages in thread
From: Tomas Vondra @ 2021-09-21 11:28 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: pgsql-hackers
On 9/21/21 3:37 AM, Justin Pryzby wrote:
> On Tue, Sep 21, 2021 at 02:15:45AM +0200, Tomas Vondra wrote:
>> On 9/15/21 10:09 PM, Justin Pryzby wrote:
>>> Memory allocation appeared be O(1) WRT the number of statistics objects, which
>>> was not expected to me. This is true in v13 (and probably back to v10).
>
> Of course I meant to say that it's O(N) and not O(1) :)
>
Sure, I got that ;-)
>> In principle we don't expect too many extended statistics on a single table,
>
> Yes, but note that expression statistics make it more reasonable to have
> multiple extended stats objects. I noticed this while testing a patch to build
> (I think) 7 stats objects on each of our current month's partitions.
> autovacuum was repeatedly killed on this vm after using using 2+GB RAM,
> probably in part because there were multiple autovacuum workers handling the
> most recent batch of inserted tables.
>
> First, I tried to determine what specifically was leaking so badly, and
> eventually converged to this patch. Maybe there's additional subcontexts which
> would be useful, but the minimum is to reset between objects.
>
Agreed.
I don't think there's much we could release, given the current design,
because we evaluate (and process) all expressions at once. We might
evaluate/process them one by one (and release the memory), but only when
no other statistics kinds are requested. That seems pretty futile.
>> These issues exist pretty much since PG10, which is where extended stats
>> were introduced, so we'll have to backpatch it. But there's no rush and I
>> don't want to interfere with rc1 at the moment.
>
> Ack that. It'd be *nice* if if the fix were included in v14.0, but I don't
> know the rules about what can change after rc1.
>
IMO this is a bugfix, and I'll get it into 14.0 (and backpatch). But I
don't want to interfere with the rc1 tagging and release, so I'll do
that later this week.
>> Attached are two patches - 0001 is your patch (seems fine, but I looked only
>> very briefly) and 0002 is the context reset I proposed.
>
> I noticed there seems to be a 3rd patch available, which might either be junk
> for testing or a cool new feature I'll hear about later ;)
>
Haha! Nope, that was just an experiment with doubling the repalloc()
sizes in functional dependencies, instead of growing them in tiny
chunks. but it does not make a measurable difference, so I haven't
included that.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: mem context is not reset between extended stats
2021-09-15 20:09 mem context is not reset between extended stats Justin Pryzby <pryzby@telsasoft.com>
2021-09-21 00:15 ` Re: mem context is not reset between extended stats Tomas Vondra <tomas.vondra@enterprisedb.com>
2021-09-21 01:37 ` Re: mem context is not reset between extended stats Justin Pryzby <pryzby@telsasoft.com>
2021-09-21 11:28 ` Re: mem context is not reset between extended stats Tomas Vondra <tomas.vondra@enterprisedb.com>
@ 2021-09-23 17:04 ` Tomas Vondra <tomas.vondra@enterprisedb.com>
0 siblings, 0 replies; 5+ messages in thread
From: Tomas Vondra @ 2021-09-23 17:04 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: pgsql-hackers
Hi,
I've pushed both of these patches, with some minor tweaks (freeing the
statistics list, and deleting the new context), and backpatched them all
the way to 10.
Thanks for the report & patch, Justin!
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2021-09-23 17:04 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-09-15 20:09 mem context is not reset between extended stats Justin Pryzby <pryzby@telsasoft.com>
2021-09-21 00:15 ` Tomas Vondra <tomas.vondra@enterprisedb.com>
2021-09-21 01:37 ` Justin Pryzby <pryzby@telsasoft.com>
2021-09-21 11:28 ` Tomas Vondra <tomas.vondra@enterprisedb.com>
2021-09-23 17:04 ` Tomas Vondra <tomas.vondra@enterprisedb.com>
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