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 <to...@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.c
index 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.c
index 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.h
index 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
