Re: ANALYZE: ERROR: tuple already updated by self

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