Thread: 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