From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Andres Freund <andres@anarazel.de>
Cc: Justin Pryzby <pryzby@telsasoft.com>
Cc: Tomas Vondra <tv@fuzzy.cz>
Cc: Dean Rasheed <dean.a.rasheed@gmail.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: ANALYZE: ERROR: tuple already updated by self
Date: Sun, 28 Jul 2019 12:15:20 +0200
Message-ID: <20190728101520.da5nptiftmsnmv7c@development> (raw)
In-Reply-To: <20190723200127.moumhmj3z4b4dohg@alap3.anarazel.de>
References: <20190618231233.GA27470@telsasoft.com>
<20190618234858.GK16019@telsasoft.com>
<20190619000837.awy6j7h45ahd4vop@alap3.anarazel.de>
<20190723200127.moumhmj3z4b4dohg@alap3.anarazel.de>
On Tue, Jul 23, 2019 at 01:01:27PM -0700, Andres Freund wrote:
>Hi,
>
>On 2019-06-18 17:08:37 -0700, Andres Freund wrote:
>> On 2019-06-18 18:48:58 -0500, Justin Pryzby wrote:
>> > Ah: the table is an inheritence parent. If I uninherit its child, there's no
>> > error during ANALYZE. MV stats on the child are ok:
>>
>> It's a "classical" inheritance parent, not a builtin-partitioning type
>> of parent, right? And it contains data?
>>
>> I assume it ought to not be too hard to come up with a reproducer
>> then...
>>
>> I think the problem is pretty plainly that for inheritance tables we'll
>> try to store extended statistics twice. And thus end up updating the
>> same row twice.
>>
>> > #6 0x0000000000588346 in do_analyze_rel (onerel=0x7fee16140de8, options=2, params=0x7ffe5b6bf8b0, va_cols=0x0, acquirefunc=0x492b4, relpages=36, inh=true, in_outer_xact=false, elevel=13) at analyze.c:627
>>
>> /* Build extended statistics (if there are any). */
>> BuildRelationExtStatistics(onerel, totalrows, numrows, rows, attr_cnt,
>> vacattrstats);
>>
>> Note that for plain statistics we a) pass down the 'inh' flag to the
>> storage function, b) stainherit is part of pg_statistics' key:
>>
>> Indexes:
>> "pg_statistic_relid_att_inh_index" UNIQUE, btree (starelid, staattnum, stainherit)
>>
>>
>> Tomas, I think at the very least extended statistics shouldn't be built
>> when building inherited stats. But going forward I think we should
>> probably have it as part of the key for pg_statistic_ext.
>
>Tomas, ping?
>
Apologies, I kinda missed this thread until now. The volume of messages
on pgsql-hackers is getting pretty insane ...
Attached is a patch fixing the error by not building extended stats for
the inh=true case (as already proposed in this thread). That's about the
simplest way to resolve this issue for v12. It should add a simple
regression test too, I guess.
To fix this properly we need to add a flag similar to stainherit
somewhere. And I've started working on that, thinking that maybe we
could do that even for v12 - it'd be yet another catversion bump, but
there's already been one since beta2 so maybe it would be OK.
But it's actually a bit more complicated than just adding a column to
the catalog, for two reasons:
1) The optimizer part has to be tweaked to pick the right object, with
the flag set to either true/false. Not trivial, but doable.
2) We don't actually have a single catalog - we have two catalogs, one
for definition, one for built statistics. The flag does not seem to be
part of the definition, and we don't know whether there will be child
rels added sometime in the future, so presumably we'd store it in the
data catalog (pg_statistic_ext_data). Which means the code gets more
complex, because right now it assumes 1:1 relationship between those
catalogs.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 2bd4e4f1499f23c27c3488d73d6fe012232936c6 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas@2ndquadrant.com>
Date: Sun, 28 Jul 2019 11:41:31 +0200
Subject: [PATCH] don't build stats for inheritance trees
---
src/backend/commands/analyze.c | 4 ++--
src/backend/statistics/extended_stats.c | 6 +++++-
src/include/statistics/statistics.h | 2 +-
3 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.cindex 8d633f2585..9dc2aaadf7 100644--- a/src/backend/commands/analyze.c+++ b/src/backend/commands/analyze.c@@ -575,8 +575,8 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
}
/* Build extended statistics (if there are any). */
- BuildRelationExtStatistics(onerel, totalrows, numrows, rows, attr_cnt,- vacattrstats);+ BuildRelationExtStatistics(onerel, inh, totalrows, numrows, rows,+ attr_cnt, vacattrstats);
}
/*
diff --git a/src/backend/statistics/extended_stats.c b/src/backend/statistics/extended_stats.cindex 23c74f7d90..8a9a396765 100644--- a/src/backend/statistics/extended_stats.c+++ b/src/backend/statistics/extended_stats.c@@ -80,7 +80,7 @@ static void statext_store(Oid relid,
* requested stats, and serializes them back into the catalog.
*/
void
-BuildRelationExtStatistics(Relation onerel, double totalrows,+BuildRelationExtStatistics(Relation onerel, bool inh, double totalrows,
int numrows, HeapTuple *rows,
int natts, VacAttrStats **vacattrstats)
{
@@ -90,6 +90,10 @@ BuildRelationExtStatistics(Relation onerel, double totalrows,
MemoryContext cxt;
MemoryContext oldcxt;
+ /* don't build stats for inheritance trees for now */+ if (inh)+ return;+
cxt = AllocSetContextCreate(CurrentMemoryContext,
"BuildRelationExtStatistics",
ALLOCSET_DEFAULT_SIZES);
diff --git a/src/include/statistics/statistics.h b/src/include/statistics/statistics.hindex cb7bc630e9..880fc32056 100644--- a/src/include/statistics/statistics.h+++ b/src/include/statistics/statistics.h@@ -97,7 +97,7 @@ extern MVNDistinct *statext_ndistinct_load(Oid mvoid);
extern MVDependencies *statext_dependencies_load(Oid mvoid);
extern MCVList *statext_mcv_load(Oid mvoid);
-extern void BuildRelationExtStatistics(Relation onerel, double totalrows,+extern void BuildRelationExtStatistics(Relation onerel, bool inh, double totalrows,
int numrows, HeapTuple *rows,
int natts, VacAttrStats **vacattrstats);
extern bool statext_is_kind_built(HeapTuple htup, char kind);
--
2.20.1
Attachments:0001-don-t-build-stats-for-inheritance-trees.patch (text/plain, 2.5K) download
From 2bd4e4f1499f23c27c3488d73d6fe012232936c6 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas@2ndquadrant.com>
Date: Sun, 28 Jul 2019 11:41:31 +0200
Subject: [PATCH] don't build stats for inheritance trees
---
src/backend/commands/analyze.c | 4 ++--
src/backend/statistics/extended_stats.c | 6 +++++-
src/include/statistics/statistics.h | 2 +-
3 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.cindex 8d633f2585..9dc2aaadf7 100644--- a/src/backend/commands/analyze.c+++ b/src/backend/commands/analyze.c@@ -575,8 +575,8 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
}
/* Build extended statistics (if there are any). */
- BuildRelationExtStatistics(onerel, totalrows, numrows, rows, attr_cnt,- vacattrstats);+ BuildRelationExtStatistics(onerel, inh, totalrows, numrows, rows,+ attr_cnt, vacattrstats);
}
/*
diff --git a/src/backend/statistics/extended_stats.c b/src/backend/statistics/extended_stats.cindex 23c74f7d90..8a9a396765 100644--- a/src/backend/statistics/extended_stats.c+++ b/src/backend/statistics/extended_stats.c@@ -80,7 +80,7 @@ static void statext_store(Oid relid,
* requested stats, and serializes them back into the catalog.
*/
void
-BuildRelationExtStatistics(Relation onerel, double totalrows,+BuildRelationExtStatistics(Relation onerel, bool inh, double totalrows,
int numrows, HeapTuple *rows,
int natts, VacAttrStats **vacattrstats)
{
@@ -90,6 +90,10 @@ BuildRelationExtStatistics(Relation onerel, double totalrows,
MemoryContext cxt;
MemoryContext oldcxt;
+ /* don't build stats for inheritance trees for now */+ if (inh)+ return;+
cxt = AllocSetContextCreate(CurrentMemoryContext,
"BuildRelationExtStatistics",
ALLOCSET_DEFAULT_SIZES);
diff --git a/src/include/statistics/statistics.h b/src/include/statistics/statistics.hindex cb7bc630e9..880fc32056 100644--- a/src/include/statistics/statistics.h+++ b/src/include/statistics/statistics.h@@ -97,7 +97,7 @@ extern MVNDistinct *statext_ndistinct_load(Oid mvoid);
extern MVDependencies *statext_dependencies_load(Oid mvoid);
extern MCVList *statext_mcv_load(Oid mvoid);
-extern void BuildRelationExtStatistics(Relation onerel, double totalrows,+extern void BuildRelationExtStatistics(Relation onerel, bool inh, double totalrows,
int numrows, HeapTuple *rows,
int natts, VacAttrStats **vacattrstats);
extern bool statext_is_kind_built(HeapTuple htup, char kind);
--
2.20.1
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tomas.vondra@2ndquadrant.com, andres@anarazel.de, pryzby@telsasoft.com, tv@fuzzy.cz, dean.a.rasheed@gmail.com
Subject: Re: ANALYZE: ERROR: tuple already updated by self
In-Reply-To: <20190728101520.da5nptiftmsnmv7c@development>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
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