Обсуждение: ANALYZE: ERROR: tuple already updated by self
A customers DB crashed due to OOM. While investigating the issue in our
report, I created MV stats, which causes this error:
ts=# CREATE STATISTICS sectors_stats (dependencies) ON site_id,sect_id FROM sectors;
CREATE STATISTICS
ts=# ANALYZE sectors;
ERROR: XX000: tuple already updated by self
LOCATION: simple_heap_update, heapam.c:4613
The issue goes away if I drop the stats object and comes back if I recreate it.
We're running 11.3 ; most of the (very few) reports from this error are from
almost 10+ years ago, running pg7.3 like.
I've taken a couple steps to resolve the issue (vacuum full and then reindex
pg_statistic and its toast and the target table, which doesn't have a toast).
I'm guessing the issue is with pg_statistic_ext, which I haven't touched.
Next step seems to be to truncate pg_statistic{,ext} and re-analyze the DB.
Does anyone want debugging/diagnostic info before I do that ?
Justin
Hi,
On 2019-06-18 18:12:33 -0500, Justin Pryzby wrote:
> A customers DB crashed due to OOM. While investigating the issue in our
> report, I created MV stats, which causes this error:
>
> ts=# CREATE STATISTICS sectors_stats (dependencies) ON site_id,sect_id FROM sectors;
> CREATE STATISTICS
> ts=# ANALYZE sectors;
> ERROR: XX000: tuple already updated by self
> LOCATION: simple_heap_update, heapam.c:4613
>
> The issue goes away if I drop the stats object and comes back if I recreate it.
>
> We're running 11.3 ; most of the (very few) reports from this error are from
> almost 10+ years ago, running pg7.3 like.
>
> I've taken a couple steps to resolve the issue (vacuum full and then reindex
> pg_statistic and its toast and the target table, which doesn't have a toast).
>
> I'm guessing the issue is with pg_statistic_ext, which I haven't touched.
>
> Next step seems to be to truncate pg_statistic{,ext} and re-analyze the DB.
>
> Does anyone want debugging/diagnostic info before I do that ?
Any chance to get a backtrace for the error?
https://wiki.postgresql.org/wiki/Getting_a_stack_trace_of_a_running_PostgreSQL_backend_on_Linux/BSD
You should be able to set a breakpoint to just the location pointed out
in the error message.
Greetings,
Andres Freund
On Tue, Jun 18, 2019 at 06:12:33PM -0500, Justin Pryzby wrote:
> A customers DB crashed due to OOM. While investigating the issue in our
> report, I created MV stats, which causes this error:
>
> ts=# CREATE STATISTICS sectors_stats (dependencies) ON site_id,sect_id FROM sectors;
> CREATE STATISTICS
> ts=# ANALYZE sectors;
> ERROR: XX000: tuple already updated by self
> LOCATION: simple_heap_update, heapam.c:4613
> I'm guessing the issue is with pg_statistic_ext, which I haven't touched.
>
> Next step seems to be to truncate pg_statistic{,ext} and re-analyze the DB.
Confirmed the issue is there.
ts=# analyze sectors;
ERROR: tuple already updated by self
ts=# begin; delete from pg_statistic_ext; analyze sectors;
BEGIN
DELETE 87
ANALYZE
On Tue, Jun 18, 2019 at 04:30:33PM -0700, Andres Freund wrote:
> Any chance to get a backtrace for the error?
Sure:
(gdb) bt
#0 errfinish (dummy=0) at elog.c:414
#1 0x000000000085e834 in elog_finish (elevel=<value optimized out>, fmt=<value optimized out>) at elog.c:1376
#2 0x00000000004b93bd in simple_heap_update (relation=0x7fee161700c8, otid=0x1fb7f44, tup=0x1fb7f40) at heapam.c:4613
#3 0x000000000051bdb7 in CatalogTupleUpdate (heapRel=0x7fee161700c8, otid=0x1fb7f44, tup=0x1fb7f40) at indexing.c:234
#4 0x000000000071e5ca in statext_store (onerel=0x7fee16140de8, totalrows=100843, numrows=100843, rows=0x1fd4028,
natts=33260176,vacattrstats=0x1fb7ef0) at extended_stats.c:344
#5 BuildRelationExtStatistics (onerel=0x7fee16140de8, totalrows=100843, numrows=100843, rows=0x1fd4028,
natts=33260176,vacattrstats=0x1fb7ef0) at extended_stats.c:130
#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
#7 0x00000000005891e1 in analyze_rel (relid=<value optimized out>, relation=0x1ea22a0, options=2,
params=0x7ffe5b6bf8b0,va_cols=0x0, in_outer_xact=false, bstrategy=0x1f38090) at analyze.c:317
#8 0x00000000005fb689 in vacuum (options=2, relations=0x1f381f0, params=0x7ffe5b6bf8b0, bstrategy=<value optimized
out>,isTopLevel=<value optimized out>) at vacuum.c:357
#9 0x00000000005fbafe in ExecVacuum (vacstmt=<value optimized out>, isTopLevel=<value optimized out>) at vacuum.c:141
#10 0x0000000000757a30 in standard_ProcessUtility (pstmt=0x1ea2410, queryString=0x1ea18c0 "ANALYZE sectors;",
context=PROCESS_UTILITY_TOPLEVEL,params=0x0, queryEnv=0x0, dest=0x1ea26d0, completionTag=0x7ffe5b6bfdf0 "")
at utility.c:670
#11 0x00007fee163a4344 in pgss_ProcessUtility (pstmt=0x1ea2410, queryString=0x1ea18c0 "ANALYZE sectors;",
context=PROCESS_UTILITY_TOPLEVEL,params=0x0, queryEnv=0x0, dest=0x1ea26d0, completionTag=0x7ffe5b6bfdf0 "")
at pg_stat_statements.c:1005
#12 0x0000000000753779 in PortalRunUtility (portal=0x1f1a8e0, pstmt=0x1ea2410, isTopLevel=<value optimized out>,
setHoldSnapshot=<valueoptimized out>, dest=0x1ea26d0, completionTag=<value optimized out>) at pquery.c:1178
#13 0x000000000075464d in PortalRunMulti (portal=0x1f1a8e0, isTopLevel=true, setHoldSnapshot=false, dest=0x1ea26d0,
altdest=0x1ea26d0,completionTag=0x7ffe5b6bfdf0 "") at pquery.c:1331
#14 0x0000000000754de8 in PortalRun (portal=0x1f1a8e0, count=9223372036854775807, isTopLevel=true, run_once=true,
dest=0x1ea26d0,altdest=0x1ea26d0, completionTag=0x7ffe5b6bfdf0 "") at pquery.c:799
#15 0x0000000000751987 in exec_simple_query (query_string=0x1ea18c0 "ANALYZE sectors;") at postgres.c:1145
#16 0x0000000000752931 in PostgresMain (argc=<value optimized out>, argv=<value optimized out>, dbname=0x1edbad8 "ts",
username=<valueoptimized out>) at postgres.c:4182
#17 0x00000000006e1ba7 in BackendRun (argc=<value optimized out>, argv=<value optimized out>) at postmaster.c:4358
#18 BackendStartup (argc=<value optimized out>, argv=<value optimized out>) at postmaster.c:4030
#19 ServerLoop (argc=<value optimized out>, argv=<value optimized out>) at postmaster.c:1707
#20 PostmasterMain (argc=<value optimized out>, argv=<value optimized out>) at postmaster.c:1380
#21 0x0000000000656210 in main (argc=3, argv=0x1e9c4d0) at main.c:228
#3 0x000000000051bdb7 in CatalogTupleUpdate (heapRel=0x7fee161700c8, otid=0x1fb7f44, tup=0x1fb7f40) at indexing.c:234
indstate = 0x1fb84a0
#4 0x000000000071e5ca in statext_store (onerel=0x7fee16140de8, totalrows=100843, numrows=100843, rows=0x1fd4028,
natts=33260176,vacattrstats=0x1fb7ef0) at extended_stats.c:344
stup = 0x1fb7f40
oldtup = 0x7fee16158530
values = {0, 0, 0, 0, 0, 0, 0, 33260544}
nulls = {true, true, true, true, true, true, true, false}
replaces = {false, false, false, false, false, false, true, true}
#5 BuildRelationExtStatistics (onerel=0x7fee16140de8, totalrows=100843, numrows=100843, rows=0x1fd4028,
natts=33260176,vacattrstats=0x1fb7ef0) at extended_stats.c:130
stat = <value optimized out>
stats = <value optimized out>
lc2 = <value optimized out>
ndistinct = <value optimized out>
dependencies = <value optimized out>
pg_stext = 0x7fee161700c8
lc = 0x1fb8290
stats = 0xfb6a172d
cxt = 0x1fb7de0
oldcxt = 0x1f6dd60
__func__ = "BuildRelationExtStatistics"
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:
ts=# CREATE STATISTICS vzw_sectors_stats (dependencies) ON site_id,sect_id FROM vzw_sectors;
CREATE STATISTICS
ts=# ANALYZE vzw_sectors;
ANALYZE
I'm not sure what the behavior is intended to be, and probably the other parent
tables I've added stats are all relkind=p.
FWIW, we also have some FKs, like:
"sectors_site_id_fkey" FOREIGN KEY (site_id) REFERENCES sites(site_id)
Justin
On Tue, Jun 18, 2019 at 06:48:58PM -0500, Justin Pryzby wrote: > On Tue, Jun 18, 2019 at 06:12:33PM -0500, Justin Pryzby wrote: > > ts=# ANALYZE sectors; > > ERROR: XX000: tuple already updated by self > > LOCATION: simple_heap_update, heapam.c:4613 > Ah: the table is an inheritence parent. If I uninherit its child, there's no > error during ANALYZE. postgres=# CREATE TABLE t(i int,j int); CREATE TABLE u() INHERITS (t); CREATE STATISTICS t_stats ON i,j FROM t; INSERT INTOt VALUES(1,1);ANALYZE t; CREATE TABLE CREATE TABLE CREATE STATISTICS INSERT 0 1 ERROR: tuple already updated by self
On Tue, Jun 18, 2019 at 4:49 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > Sure: > > (gdb) bt > #0 errfinish (dummy=0) at elog.c:414 > #1 0x000000000085e834 in elog_finish (elevel=<value optimized out>, fmt=<value optimized out>) at elog.c:1376 > #2 0x00000000004b93bd in simple_heap_update (relation=0x7fee161700c8, otid=0x1fb7f44, tup=0x1fb7f40) at heapam.c:4613 > #3 0x000000000051bdb7 in CatalogTupleUpdate (heapRel=0x7fee161700c8, otid=0x1fb7f44, tup=0x1fb7f40) at indexing.c:234 It might be interesting to set a breakpoint within heap_update(), which is called by simple_heap_update() --technically, this is where the reported failure occurs. From there, you could send an image of the page to the list by following the procedure described here: https://wiki.postgresql.org/wiki/Getting_a_stack_trace_of_a_running_PostgreSQL_backend_on_Linux/BSD#Dumping_a_page_image_from_within_GDB You'll have to hit "next" a few times, until heap_update()'s "page" variable is initialized. -- Peter Geoghegan
Hi,
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.
Greetings,
Andres Freund
Hi, On 2019-06-18 17:00:09 -0700, Peter Geoghegan wrote: > On Tue, Jun 18, 2019 at 4:49 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > > Sure: > > > > (gdb) bt > > #0 errfinish (dummy=0) at elog.c:414 > > #1 0x000000000085e834 in elog_finish (elevel=<value optimized out>, fmt=<value optimized out>) at elog.c:1376 > > #2 0x00000000004b93bd in simple_heap_update (relation=0x7fee161700c8, otid=0x1fb7f44, tup=0x1fb7f40) at heapam.c:4613 > > #3 0x000000000051bdb7 in CatalogTupleUpdate (heapRel=0x7fee161700c8, otid=0x1fb7f44, tup=0x1fb7f40) at indexing.c:234 > > It might be interesting to set a breakpoint within heap_update(), > which is called by simple_heap_update() --technically, this is where > the reported failure occurs. From there, you could send an image of > the page to the list by following the procedure described here: > > https://wiki.postgresql.org/wiki/Getting_a_stack_trace_of_a_running_PostgreSQL_backend_on_Linux/BSD#Dumping_a_page_image_from_within_GDB > > You'll have to hit "next" a few times, until heap_update()'s "page" > variable is initialized. Hm, what are you hoping to glean by doing so? Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> 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.
They shouldn't be the same row though. If we're to try to capture
ext-stats on inheritance trees --- and I think that's likely a good
idea --- then we need a bool corresponding to pg_statistic's stainherit
as part of pg_statistic_ext's primary key.
Since there is no such bool there now, and I assume that nobody wants
yet another pg_statistic_ext-driven catversion bump for v12, the only
fix is to get the stats machinery to not compute or store such stats.
For now. But I think we ought to change that in v13.
regards, tom lane
On Tue, Jun 18, 2019 at 5:09 PM Andres Freund <andres@anarazel.de> wrote: > > It might be interesting to set a breakpoint within heap_update(), > > which is called by simple_heap_update() --technically, this is where > > the reported failure occurs. From there, you could send an image of > > the page to the list by following the procedure described here: > Hm, what are you hoping to glean by doing so? Nothing in particular. I see no reason to assume that we know what that looks like, though. It's easy to check. -- Peter Geoghegan
On Tue, Jun 18, 2019 at 06:48:58PM -0500, Justin Pryzby wrote:
> On Tue, Jun 18, 2019 at 06:12:33PM -0500, Justin Pryzby wrote:
> > A customers DB crashed due to OOM. While investigating the issue in our
> > report, I created MV stats, which causes this error:
> >
> > ts=# CREATE STATISTICS sectors_stats (dependencies) ON site_id,sect_id FROM sectors;
> > CREATE STATISTICS
> > ts=# ANALYZE sectors;
> > ERROR: XX000: tuple already updated by self
> > LOCATION: simple_heap_update, heapam.c:4613
>
> > I'm guessing the issue is with pg_statistic_ext, which I haven't touched.
> >
> > Next step seems to be to truncate pg_statistic{,ext} and re-analyze the DB.
>
> Confirmed the issue is there.
>
> ts=# analyze sectors;
> ERROR: tuple already updated by self
> ts=# begin; delete from pg_statistic_ext; analyze sectors;
> BEGIN
> DELETE 87
> ANALYZE
Why this works seems to me to be unexplained..
Justin
Hi,
On June 18, 2019 5:38:34 PM PDT, Justin Pryzby <pryzby@telsasoft.com> wrote:
>On Tue, Jun 18, 2019 at 06:48:58PM -0500, Justin Pryzby wrote:
>> On Tue, Jun 18, 2019 at 06:12:33PM -0500, Justin Pryzby wrote:
>> > A customers DB crashed due to OOM. While investigating the issue
>in our
>> > report, I created MV stats, which causes this error:
>> >
>> > ts=# CREATE STATISTICS sectors_stats (dependencies) ON
>site_id,sect_id FROM sectors;
>> > CREATE STATISTICS
>> > ts=# ANALYZE sectors;
>> > ERROR: XX000: tuple already updated by self
>> > LOCATION: simple_heap_update, heapam.c:4613
>>
>> > I'm guessing the issue is with pg_statistic_ext, which I haven't
>touched.
>> >
>> > Next step seems to be to truncate pg_statistic{,ext} and re-analyze
>the DB.
>>
>> Confirmed the issue is there.
>>
>> ts=# analyze sectors;
>> ERROR: tuple already updated by self
>> ts=# begin; delete from pg_statistic_ext; analyze sectors;
>> BEGIN
>> DELETE 87
>> ANALYZE
>
>Why this works seems to me to be unexplained..
There's no extended stats to compute after that, thus we don't try to update the extended stats twice.
Address
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
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? Greetings, Andres Freund
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
Вложения
Hi, On 2019-07-28 12:15:20 +0200, Tomas Vondra wrote: > 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. Doesn't this also apply to v11? Greetings, Andres Freund
On Sun, Jul 28, 2019 at 09:42:44AM -0700, Andres Freund wrote: >Hi, > >On 2019-07-28 12:15:20 +0200, Tomas Vondra wrote: >> 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. > >Doesn't this also apply to v11? > AFAICS it applies to 10+ versions, because that's where extended stats were introduced. We certainly can't mess with catalogs there, so this is about the only backpatchable fix I can think of. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi, On 2019-07-28 21:21:51 +0200, Tomas Vondra wrote: > AFAICS it applies to 10+ versions, because that's where extended stats > were introduced. We certainly can't mess with catalogs there, so this is > about the only backpatchable fix I can think of. AFAIU the inh version wouldn't be used anyway, and this has never worked. So I think it's imo fine to fix it that way for < master. For master we should have something better, even if perhaps not immediately. - Andres
On Sun, 28 Jul 2019 at 11:15, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote: > > 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. > Seems like a reasonable thing to do for 10, 11 and possibly also 12 (actually, as you noted, I think it's the only thing that can be done for 10 and 11). > 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. > Yeah, I think that makes sense, if it's not too hard. Since v12 is where the stats definition is split out from the stats data, this might work out quite neatly, since the inh flag would apply only to the stats data. > 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. > Isn't it just a matter of passing the inh flag to get_relation_statistics() from get_relation_info(), so then the optimiser would get the right kind of stats data, depending on whether or not inheritance was requested in the query. Regards, Dean
On Mon, Jul 29, 2019 at 10:15:36AM +0100, Dean Rasheed wrote: >On Sun, 28 Jul 2019 at 11:15, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote: >> >> 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. >> > >Seems like a reasonable thing to do for 10, 11 and possibly also 12 >(actually, as you noted, I think it's the only thing that can be done >for 10 and 11). > OK, will do. >> 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. >> > >Yeah, I think that makes sense, if it's not too hard. Since v12 is >where the stats definition is split out from the stats data, this >might work out quite neatly, since the inh flag would apply only to >the stats data. > Agreed, we need to add the inh flag to the pg_statistic_ext_data catalog. The trouble is this makes the maintenance somewhat more complicated, because we suddenly don't have 1:1 mapping :-( But if we want to address this in master only, I think that's fine. >> 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. >> > >Isn't it just a matter of passing the inh flag to >get_relation_statistics() from get_relation_info(), so then the >optimiser would get the right kind of stats data, depending on whether >or not inheritance was requested in the query. > Yes, you're right. I've only skimmed how the existing code uses the inh flag (for regular stats) and it seemed somewhat more complex, but you're right for extended stats it'd be much simpler. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sun, Jul 28, 2019 at 09:53:20PM -0700, Andres Freund wrote: >Hi, > >On 2019-07-28 21:21:51 +0200, Tomas Vondra wrote: >> AFAICS it applies to 10+ versions, because that's where extended stats >> were introduced. We certainly can't mess with catalogs there, so this is >> about the only backpatchable fix I can think of. > >AFAIU the inh version wouldn't be used anyway, and this has never >worked. So I think it's imo fine to fix it that way for < master. For >master we should have something better, even if perhaps not immediately. > Agreed. I'll get the simple fix committed soon, and put a TODO on my list for pg13. thanks -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, Jul 29, 2019 at 12:18:33PM +0200, Tomas Vondra wrote: >On Sun, Jul 28, 2019 at 09:53:20PM -0700, Andres Freund wrote: >>Hi, >> >>On 2019-07-28 21:21:51 +0200, Tomas Vondra wrote: >>>AFAICS it applies to 10+ versions, because that's where extended stats >>>were introduced. We certainly can't mess with catalogs there, so this is >>>about the only backpatchable fix I can think of. >> >>AFAIU the inh version wouldn't be used anyway, and this has never >>worked. So I think it's imo fine to fix it that way for < master. For >>master we should have something better, even if perhaps not immediately. >> > >Agreed. I'll get the simple fix committed soon, and put a TODO on my >list for pg13. > I've pushed the simple fix - I've actually simplified it a bit further by simply not calling the BuildRelationExtStatistics() at all when inh=true, instead of passing the flag to BuildRelationExtStatistics() and making the decision there. The function is part of public API, so this would be an ABI break (although it's unlikely anyone else is calling the function). regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services