Thread

Commits

Describe (auto-)analyze behavior for partitioned tables
- 1b5617eb844c 14.0 landed
Add comment about extract_autovac_opts not holding lock
- 7c298c6573a0 14.0 landed
Set pg_class.reltuples for partitioned tables
- 0e69f705cc1a 14.0 landed
autovacuum: handle analyze for partitioned tables
- 0827e8af70f4 14.0 cited

pgsql: autovacuum: handle analyze for partitioned tables

Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-04-08T05:20:39Z

autovacuum: handle analyze for partitioned tables

Previously, autovacuum would completely ignore partitioned tables, which
is not good regarding analyze -- failing to analyze those tables means
poor plans may be chosen.  Make autovacuum aware of those tables by
propagating "changes since analyze" counts from the leaf partitions up
the partitioning hierarchy.

This also introduces necessary reloptions support for partitioned tables
(autovacuum_enabled, autovacuum_analyze_scale_factor,
autovacuum_analyze_threshold).  It's unclear how best to document this
aspect.

Author: Yuzuko Hosoya <yuzukohosoya@gmail.com>
Reviewed-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Reviewed-by: Tomas Vondra <tomas.vondra@enterprisedb.com>
Reviewed-by: Álvaro Herrera <alvherre@alvh.no-ip.org>
Discussion: https://postgr.es/m/CAKkQ508_PwVgwJyBY=0Lmkz90j8CmWNPUxgHvCUwGhMrouz6UA@mail.gmail.com

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/0827e8af70f4653ba17ed773f123a60eadd9f9c9

Modified Files
--------------
src/backend/access/common/reloptions.c |  15 +++--
src/backend/catalog/system_views.sql   |   4 +-
src/backend/commands/analyze.c         |  40 ++++++++----
src/backend/postmaster/autovacuum.c    | 105 +++++++++++++++++++++++++++++---
src/backend/postmaster/pgstat.c        | 108 ++++++++++++++++++++++++++++++---
src/include/pgstat.h                   |  25 +++++++-
src/test/regress/expected/rules.out    |   4 +-
7 files changed, 257 insertions(+), 44 deletions(-)

Re: pgsql: autovacuum: handle analyze for partitioned tables

Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-08T06:16:30Z

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> autovacuum: handle analyze for partitioned tables

Looks like this has issues under EXEC_BACKEND:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=culicidae&dt=2021-04-08%2005%3A50%3A08

			regards, tom lane

Re: pgsql: autovacuum: handle analyze for partitioned tables

Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-04-08T15:19:37Z

On 2021-Apr-08, Tom Lane wrote:

> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > autovacuum: handle analyze for partitioned tables
> 
> Looks like this has issues under EXEC_BACKEND:
> 
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=culicidae&dt=2021-04-08%2005%3A50%3A08

Hmm, I couldn't reproduce this under EXEC_BACKEND or otherwise, but I
think this is unrelated to that, but rather a race condition.

The backtrace saved by buildfarm is:

Program terminated with signal SIGSEGV, Segmentation fault.
#0  relation_needs_vacanalyze (relid=relid@entry=43057, relopts=relopts@entry=0x0, classForm=classForm@entry=0x7e000501eef0, tabentry=0x5611ec71b030, effective_multixact_freeze_max_age=effective_multixact_freeze_max_age@entry=400000000, dovacuum=dovacuum@entry=0x7ffd78cc4ee0, doanalyze=0x7ffd78cc4ee1, wraparound=0x7ffd78cc4ee2) at /mnt/resource/andres/bf/culicidae/HEAD/pgsql.build/../pgsql/src/backend/postmaster/autovacuum.c:3237
3237					childclass = (Form_pg_class) GETSTRUCT(childtuple);
#0  relation_needs_vacanalyze (relid=relid@entry=43057, relopts=relopts@entry=0x0, classForm=classForm@entry=0x7e000501eef0, tabentry=0x5611ec71b030, effective_multixact_freeze_max_age=effective_multixact_freeze_max_age@entry=400000000, dovacuum=dovacuum@entry=0x7ffd78cc4ee0, doanalyze=0x7ffd78cc4ee1, wraparound=0x7ffd78cc4ee2) at /mnt/resource/andres/bf/culicidae/HEAD/pgsql.build/../pgsql/src/backend/postmaster/autovacuum.c:3237
#1  0x00005611eb09fc91 in do_autovacuum () at /mnt/resource/andres/bf/culicidae/HEAD/pgsql.build/../pgsql/src/backend/postmaster/autovacuum.c:2168
#2  0x00005611eb0a0f8b in AutoVacWorkerMain (argc=argc@entry=1, argv=argv@entry=0x5611ec61f1e0) at /mnt/resource/andres/bf/culicidae/HEAD/pgsql.build/../pgsql/src/backend/postmaster/autovacuum.c:1715

the code in question is:

			children = find_all_inheritors(relid, AccessShareLock, NULL);

			foreach(lc, children)
			{
				Oid			childOID = lfirst_oid(lc);
				HeapTuple	childtuple;
				Form_pg_class childclass;

				childtuple = SearchSysCache1(RELOID, ObjectIdGetDatum(childOID));
				childclass = (Form_pg_class) GETSTRUCT(childtuple);

Evidently SearchSysCache must be returning NULL, but how come that
happens, when we have acquired lock on the rel during
find_all_inheritors?

I would suggest that we do not take lock here at all, and just skip the
rel if SearchSysCache returns empty, as in the attached.  Still, I am
baffled about this crash.

-- 
Álvaro Herrera       Valdivia, Chile
"Oh, great altar of passive entertainment, bestow upon me thy discordant images
at such speed as to render linear thought impossible" (Calvin a la TV)

Re: pgsql: autovacuum: handle analyze for partitioned tables

Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-08T17:57:45Z

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2021-Apr-08, Tom Lane wrote:
>> Looks like this has issues under EXEC_BACKEND:
>> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=culicidae&dt=2021-04-08%2005%3A50%3A08

> Hmm, I couldn't reproduce this under EXEC_BACKEND or otherwise, but I
> think this is unrelated to that, but rather a race condition.

Yeah.  I hit this on another machine that isn't using EXEC_BACKEND,
and I concur it looks more like a race condition.  I think the problem
is that autovacuum is calling find_all_inheritors() on a relation it
has no lock on, contrary to that function's API spec.  find_all_inheritors
assumes the OID it's given is valid and locked, and adds it to the
result list automatically.  Then it looks for children, and won't find
any in the race case where somebody else just dropped the table.
So we come back to relation_needs_vacanalyze with a list of just the
original rel OID, and since this loop believes that every syscache fetch
it does will succeed, kaboom.

I do not think it is sane to do find_all_inheritors() with no lock,
so I'd counsel doing something about that rather than band-aiding the
symptom.  On the other hand, it's also not really okay not to have
an if-test-and-elog after the SearchSysCache call.  "A cache lookup
cannot fail" is not an acceptable assumption in my book.

BTW, another thing that looks like a race condition is the
extract_autovac_opts() call that is done a little bit earlier,
also without lock.  I think this is actually safe, but it's ONLY
safe because we resisted the calls by certain people to add a
toast table to pg_class.  Otherwise, fetching reloptions could
have involved a toast pointer dereference, and it would then be
racy whether the toasted data was still there.  As-is, even if
the pg_class row we're looking at has been deleted, we can safely
disassemble its reloptions.  I think this matter is deserving
of a comment at least.

			regards, tom lane

Re: pgsql: autovacuum: handle analyze for partitioned tables

Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-04-08T18:35:51Z

On 2021-Apr-08, Tom Lane wrote:

> Yeah.  I hit this on another machine that isn't using EXEC_BACKEND,
> and I concur it looks more like a race condition.  I think the problem
> is that autovacuum is calling find_all_inheritors() on a relation it
> has no lock on, contrary to that function's API spec.  find_all_inheritors
> assumes the OID it's given is valid and locked, and adds it to the
> result list automatically.  Then it looks for children, and won't find
> any in the race case where somebody else just dropped the table.

Hmm.  Autovacuum tries hard to avoid grabbing locks on relations until
really needed (at vacuum/analyze time), which is why all these tests
only use data that can be found in the pg_class rows and pgstat entries.
So I tend to think that my initial instinct was the better direction: we
should not be doing any find_all_inheritors() here at all, but instead
rely on pg_class.reltuples to be set for the partitioned table.

I'll give that another look.  Most places already assume that reltuples
isn't set for a partitioned table, so they shouldn't care.  I wonder,
though, whether we should set relpages to some value other than 0 or -1.
(I'm inclined not to, since autovacuum does not use it.)

-- 
Álvaro Herrera       Valdivia, Chile

Re: pgsql: autovacuum: handle analyze for partitioned tables

Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-08T18:47:01Z

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2021-Apr-08, Tom Lane wrote:
>> Yeah.  I hit this on another machine that isn't using EXEC_BACKEND,
>> and I concur it looks more like a race condition.  I think the problem
>> is that autovacuum is calling find_all_inheritors() on a relation it
>> has no lock on, contrary to that function's API spec.

> Hmm.  Autovacuum tries hard to avoid grabbing locks on relations until
> really needed (at vacuum/analyze time), which is why all these tests
> only use data that can be found in the pg_class rows and pgstat entries.

Yeah, I was worried about that.

> So I tend to think that my initial instinct was the better direction: we
> should not be doing any find_all_inheritors() here at all, but instead
> rely on pg_class.reltuples to be set for the partitioned table.

+1

			regards, tom lane

Re: pgsql: autovacuum: handle analyze for partitioned tables

Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-04-08T20:11:49Z

On 2021-Apr-08, Tom Lane wrote:

> > So I tend to think that my initial instinct was the better direction: we
> > should not be doing any find_all_inheritors() here at all, but instead
> > rely on pg_class.reltuples to be set for the partitioned table.
> 
> +1

This patch does that.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W
"I dream about dreams about dreams", sang the nightingale
under the pale moon (Sandman)

Re: pgsql: autovacuum: handle analyze for partitioned tables

Zhihong Yu <zyu@yugabyte.com> — 2021-04-08T20:25:24Z

On Thu, Apr 8, 2021 at 1:12 PM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

> On 2021-Apr-08, Tom Lane wrote:
>
> > > So I tend to think that my initial instinct was the better direction:
> we
> > > should not be doing any find_all_inheritors() here at all, but instead
> > > rely on pg_class.reltuples to be set for the partitioned table.
> >
> > +1
>
> This patch does that.
>
> --
> Álvaro Herrera                            39°49'30"S 73°17'W
> "I dream about dreams about dreams", sang the nightingale
> under the pale moon (Sandman)
>

Hi,
Within truncate_update_partedrel_stats(), dirty is declared within the loop.
+       if (rd_rel->reltuples != 0)
+       {
...
+       if (dirty)

The two if blocks can be merged. The variable dirty can be dropped.

Cheers

Re: pgsql: autovacuum: handle analyze for partitioned tables

Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-04-08T21:29:51Z

On 2021-Apr-08, Zhihong Yu wrote:

> Hi,
> Within truncate_update_partedrel_stats(), dirty is declared within the loop.
> +       if (rd_rel->reltuples != 0)
> +       {
> ...
> +       if (dirty)
> 
> The two if blocks can be merged. The variable dirty can be dropped.

Hi, thanks for reviewing.  Yes, evidently I copied vac_update_relstats
too closely -- that boolean is not necessary.

-- 
Álvaro Herrera       Valdivia, Chile

Re: pgsql: autovacuum: handle analyze for partitioned tables

Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-09T11:53:45Z

Could we get this pushed sooner rather than later?  The buildfarm
is showing a wide variety of intermittent failures on HEAD, and it's
hard to tell how many of them trace to this one bug.

			regards, tom lane

Re: pgsql: autovacuum: handle analyze for partitioned tables

Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-04-09T15:54:30Z

On 2021-Apr-09, Tom Lane wrote:

> Could we get this pushed sooner rather than later?  The buildfarm
> is showing a wide variety of intermittent failures on HEAD, and it's
> hard to tell how many of them trace to this one bug.

Pushed now, thanks.

-- 
Álvaro Herrera       Valdivia, Chile
"Digital and video cameras have this adjustment and film cameras don't for the
same reason dogs and cats lick themselves: because they can."   (Ken Rockwell)

Re: pgsql: autovacuum: handle analyze for partitioned tables

Robert Haas <robertmhaas@gmail.com> — 2021-04-09T20:39:47Z

On Fri, Apr 9, 2021 at 11:54 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> On 2021-Apr-09, Tom Lane wrote:
> > Could we get this pushed sooner rather than later?  The buildfarm
> > is showing a wide variety of intermittent failures on HEAD, and it's
> > hard to tell how many of them trace to this one bug.
>
> Pushed now, thanks.

Does this need to worry about new partitions getting attached to a
partitioned table, or old ones getting detached? (Maybe it does
already, not sure.)

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: pgsql: autovacuum: handle analyze for partitioned tables

Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-04-09T21:31:55Z

On 2021-Apr-09, Robert Haas wrote:

> Does this need to worry about new partitions getting attached to a
> partitioned table, or old ones getting detached? (Maybe it does
> already, not sure.)

Good question.  It does not.

I suppose you could just let that happen automatically -- I mean, next
time the partitioned table is analyzed, it'll scan all attached
partitions.  But if no tuples are modified afterwards in existing
partitions (a common scenario), and the newly attached partition
contains lots of rows, then only future rows in the newly attached
partition would affect the stats of the partitioned table, and it could
be a long time before that causes an analyze on the partitioned table to
occur.

Maybe a way to attack this is to send a the "anl_ancestors" message to
the collector on attach and detach, adding a new flag ("is
attach/detach"), which indicates to add not only "changes_since_analyze
- changes_since_analyze_reported", but also "n_live_tuples".

-- 
Álvaro Herrera                            39°49'30"S 73°17'W
Essentially, you're proposing Kevlar shoes as a solution for the problem
that you want to walk around carrying a loaded gun aimed at your foot.
(Tom Lane)

Re: pgsql: autovacuum: handle analyze for partitioned tables

Justin Pryzby <pryzby@telsasoft.com> — 2021-04-09T21:45:08Z

On Fri, Apr 09, 2021 at 05:31:55PM -0400, Alvaro Herrera wrote:
> On 2021-Apr-09, Robert Haas wrote:
> 
> > Does this need to worry about new partitions getting attached to a
> > partitioned table, or old ones getting detached? (Maybe it does
> > already, not sure.)
> 
> Good question.  It does not.

I think there's probably cases where this is desirable, and cases where it's
undesirable, so I don't think it's necessarily a problem.

One data point: we do DETACH/ATTACH tables during normal operation, before
type-promoting ALTERs, to avoid worst-case disk use, and to avoid locking the
table for a long time.  It'd be undesirable (but maybe of no great consequence)
to trigger an ALTER when we DETACH them, since we'll re-ATTACH it shortly
afterwards.

However, I think DROP should be handled ?

-- 
Justin

Re: pgsql: autovacuum: handle analyze for partitioned tables

Tomas Vondra <tomas.vondra@enterprisedb.com> — 2021-04-09T21:53:08Z


On 4/9/21 11:45 PM, Justin Pryzby wrote:
> On Fri, Apr 09, 2021 at 05:31:55PM -0400, Alvaro Herrera wrote:
>> On 2021-Apr-09, Robert Haas wrote:
>>
>>> Does this need to worry about new partitions getting attached to a
>>> partitioned table, or old ones getting detached? (Maybe it does
>>> already, not sure.)
>>
>> Good question.  It does not.
> 
> I think there's probably cases where this is desirable, and cases where it's
> undesirable, so I don't think it's necessarily a problem.
> 
> One data point: we do DETACH/ATTACH tables during normal operation, before
> type-promoting ALTERs, to avoid worst-case disk use, and to avoid locking the
> table for a long time.  It'd be undesirable (but maybe of no great consequence)
> to trigger an ALTER when we DETACH them, since we'll re-ATTACH it shortly
> afterwards.
> 
> However, I think DROP should be handled ?
> 

IMHO we should prefer the default behavior which favors having updated
statistics, and maybe have a way to override it for individual commands.
So ATTACH would update changes_since_analyze by default, but it would be
possible to disable that.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: pgsql: autovacuum: handle analyze for partitioned tables

Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-04-09T22:16:59Z

On 2021-Apr-09, Justin Pryzby wrote:

> One data point: we do DETACH/ATTACH tables during normal operation, before
> type-promoting ALTERs, to avoid worst-case disk use, and to avoid locking the
> table for a long time.  It'd be undesirable (but maybe of no great consequence)
> to trigger an ALTER when we DETACH them, since we'll re-ATTACH it shortly
> afterwards.

You mean to trigger an ANALYZE, not to trigger an ALTER, right?

I think I agree with Tomas: we should do it by default, and offer some
way to turn that off.  I suppose a new reloptions, solely for
partitioned tables, would be the way to do it.

> However, I think DROP should be handled ?

DROP of a partition? ... I would think it should do the same as DETACH,
right?  Inform that however many rows the partition had, are now changed
in ancestors.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W
"Aprender sin pensar es inútil; pensar sin aprender, peligroso" (Confucio)

Re: pgsql: autovacuum: handle analyze for partitioned tables

Justin Pryzby <pryzby@telsasoft.com> — 2021-04-09T22:29:58Z

On Fri, Apr 09, 2021 at 06:16:59PM -0400, Alvaro Herrera wrote:
> On 2021-Apr-09, Justin Pryzby wrote:
> 
> > One data point: we do DETACH/ATTACH tables during normal operation, before
> > type-promoting ALTERs, to avoid worst-case disk use, and to avoid locking the
> > table for a long time.  It'd be undesirable (but maybe of no great consequence)
> > to trigger an ALTER when we DETACH them, since we'll re-ATTACH it shortly
> > afterwards.
> 
> You mean to trigger an ANALYZE, not to trigger an ALTER, right?

Oops, right.  It's slightly undesirable for a DETACH to cause an ANALYZE.

> I think I agree with Tomas: we should do it by default, and offer some
> way to turn that off.  I suppose a new reloptions, solely for
> partitioned tables, would be the way to do it.
> 
> > However, I think DROP should be handled ?
> 
> DROP of a partition? ... I would think it should do the same as DETACH,
> right?  Inform that however many rows the partition had, are now changed
> in ancestors.

Yes, drop of an (attached) partition.  The case for DROP is clear, since it
was clearly meant to go away forever.  The case for DETACH seems somewhat less
clear.

The current behavior of pg_dump/restore (since 33a53130a) is to CREATE+ATTACH,
so there's an argument that if DROPping the partition counts towards the
parent's analyze, then so should CREATE+ATTACH.

-- 
Justin

Re: pgsql: autovacuum: handle analyze for partitioned tables

Andres Freund <andres@anarazel.de> — 2021-04-09T23:47:28Z

Hi,

On 2021-04-09 11:54:30 -0400, Alvaro Herrera wrote:
> On 2021-Apr-09, Tom Lane wrote:
>
> > Could we get this pushed sooner rather than later?  The buildfarm
> > is showing a wide variety of intermittent failures on HEAD, and it's
> > hard to tell how many of them trace to this one bug.
>
> Pushed now, thanks.

I assume this is also the likely explanation for / fix for:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink&dt=2021-04-08%2016%3A03%3A03

==3500389== VALGRINDERROR-BEGIN
==3500389== Invalid read of size 8
==3500389==    at 0x4EC4B8: relation_needs_vacanalyze (autovacuum.c:3237)
==3500389==    by 0x4EE0AF: do_autovacuum (autovacuum.c:2168)
==3500389==    by 0x4EEEA8: AutoVacWorkerMain (autovacuum.c:1715)
==3500389==    by 0x4EEF7F: StartAutoVacWorker (autovacuum.c:1500)
==3500389==    by 0x4FD2E4: StartAutovacuumWorker (postmaster.c:5539)
==3500389==    by 0x4FE50A: sigusr1_handler (postmaster.c:5243)
==3500389==    by 0x4A6513F: ??? (in /usr/lib/x86_64-linux-gnu/libpthread-2.31.so)
==3500389==    by 0x4DCA865: select (select.c:41)
==3500389==    by 0x4FEB75: ServerLoop (postmaster.c:1701)
==3500389==    by 0x4FFE52: PostmasterMain (postmaster.c:1409)
==3500389==    by 0x442563: main (main.c:209)
==3500389==  Address 0x10 is not stack'd, malloc'd or (recently) free'd
==3500389==
==3500389== VALGRINDERROR-END
==3500389==
==3500389== Process terminating with default action of signal 11 (SIGSEGV): dumping core
==3500389==  Access not within mapped region at address 0x10
==3500389==    at 0x4EC4B8: relation_needs_vacanalyze (autovacuum.c:3237)
==3500389==    by 0x4EE0AF: do_autovacuum (autovacuum.c:2168)
==3500389==    by 0x4EEEA8: AutoVacWorkerMain (autovacuum.c:1715)
==3500389==    by 0x4EEF7F: StartAutoVacWorker (autovacuum.c:1500)
==3500389==    by 0x4FD2E4: StartAutovacuumWorker (postmaster.c:5539)
==3500389==    by 0x4FE50A: sigusr1_handler (postmaster.c:5243)
==3500389==    by 0x4A6513F: ??? (in /usr/lib/x86_64-linux-gnu/libpthread-2.31.so)
==3500389==    by 0x4DCA865: select (select.c:41)
==3500389==    by 0x4FEB75: ServerLoop (postmaster.c:1701)
==3500389==    by 0x4FFE52: PostmasterMain (postmaster.c:1409)
==3500389==    by 0x442563: main (main.c:209)
==3500389==  If you believe this happened as a result of a stack
==3500389==  overflow in your program's main thread (unlikely but
==3500389==  possible), you can try to increase the size of the
==3500389==  main thread stack using the --main-stacksize= flag.
==3500389==  The main thread stack size used in this run was 8388608.

Greetings,

Andres Freund

Re: pgsql: autovacuum: handle analyze for partitioned tables

Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-04-10T00:01:04Z

Hello

On 2021-Apr-09, Andres Freund wrote:

> I assume this is also the likely explanation for / fix for:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink&dt=2021-04-08%2016%3A03%3A03
> 
> ==3500389== VALGRINDERROR-BEGIN
> ==3500389== Invalid read of size 8
> ==3500389==    at 0x4EC4B8: relation_needs_vacanalyze (autovacuum.c:3237)
> ==3500389==    by 0x4EE0AF: do_autovacuum (autovacuum.c:2168)
> ==3500389==    by 0x4EEEA8: AutoVacWorkerMain (autovacuum.c:1715)
> ==3500389==    by 0x4EEF7F: StartAutoVacWorker (autovacuum.c:1500)
> ==3500389==    by 0x4FD2E4: StartAutovacuumWorker (postmaster.c:5539)

Hmm, I didn't try to reproduce this, but yeah it sounds quite likely
that it's the same issue -- line 3237 is the GETSTRUCT call where the
other one was crashing, which is now gone.

Thanks for pointing it out,

-- 
Álvaro Herrera       Valdivia, Chile

Re: pgsql: autovacuum: handle analyze for partitioned tables

Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-10T02:52:05Z

Andres Freund <andres@anarazel.de> writes:
> On 2021-04-09 11:54:30 -0400, Alvaro Herrera wrote:
>> Pushed now, thanks.

> I assume this is also the likely explanation for / fix for:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink&dt=2021-04-08%2016%3A03%3A03

> ==3500389== VALGRINDERROR-BEGIN
> ==3500389== Invalid read of size 8
> ==3500389==    at 0x4EC4B8: relation_needs_vacanalyze (autovacuum.c:3237)

Yeah, looks like the same thing to me; it's the same line that was
crashing in the non-valgrind reports.

			regards, tom lane

Re: pgsql: autovacuum: handle analyze for partitioned tables

Tomas Vondra <tomas.vondra@enterprisedb.com> — 2021-04-10T21:22:53Z

On 4/10/21 12:29 AM, Justin Pryzby wrote:
> On Fri, Apr 09, 2021 at 06:16:59PM -0400, Alvaro Herrera wrote:
>> On 2021-Apr-09, Justin Pryzby wrote:
>>
>>> One data point: we do DETACH/ATTACH tables during normal operation, before
>>> type-promoting ALTERs, to avoid worst-case disk use, and to avoid locking the
>>> table for a long time.  It'd be undesirable (but maybe of no great consequence)
>>> to trigger an ALTER when we DETACH them, since we'll re-ATTACH it shortly
>>> afterwards.
>>
>> You mean to trigger an ANALYZE, not to trigger an ALTER, right?
> 
> Oops, right.  It's slightly undesirable for a DETACH to cause an ANALYZE.
> 
>> I think I agree with Tomas: we should do it by default, and offer some
>> way to turn that off.  I suppose a new reloptions, solely for
>> partitioned tables, would be the way to do it.
>>
>>> However, I think DROP should be handled ?
>>
>> DROP of a partition? ... I would think it should do the same as DETACH,
>> right?  Inform that however many rows the partition had, are now changed
>> in ancestors.
> 
> Yes, drop of an (attached) partition.  The case for DROP is clear, since it
> was clearly meant to go away forever.  The case for DETACH seems somewhat less
> clear.
> 
> The current behavior of pg_dump/restore (since 33a53130a) is to CREATE+ATTACH,
> so there's an argument that if DROPping the partition counts towards the
> parent's analyze, then so should CREATE+ATTACH.
> 

I think it's tricky to "optimize" the behavior after ATTACH/DETACH. I'd
argue that in principle, we should aim to keep accurate statistics,  so
ATTACH should be treated as insert of all rows, and DETACH should be
treated as delete of all rows. Se for the purpose of ANALYZE, we should
propagate reltuples as changes_since_analyze after ATTACH/DETACH.

Yes, it may result in more frequent ANALYZE on the parent, but I think
that's necessary. Repeated attach/detach of the same partition may bloat
the value, but I guess that's an example of "If it hurts don't do it."

What I think we might do is offer some light-weight analyze variant,
e.g. based on the merging of statistics (I've posted a PoC patch a
couple days ago.). That would make the ANALYZEs on parent much cheaper,
so those "unnecessary" analyzes would not be an issue.

regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: pgsql: autovacuum: handle analyze for partitioned tables

Justin Pryzby <pryzby@telsasoft.com> — 2021-04-11T01:55:37Z

On Thu, Apr 08, 2021 at 04:11:49PM -0400, Alvaro Herrera wrote:
> On 2021-Apr-08, Tom Lane wrote:
> 
> > > So I tend to think that my initial instinct was the better direction: we
> > > should not be doing any find_all_inheritors() here at all, but instead
> > > rely on pg_class.reltuples to be set for the partitioned table.
> > 
> > +1
> 
> This patch does that.

|commit 0e69f705cc1a3df273b38c9883fb5765991e04fe
|Author: Alvaro Herrera <alvherre@alvh.no-ip.org>
|Date:   Fri Apr 9 11:29:08 2021 -0400
|
|    Set pg_class.reltuples for partitioned tables
|    
|    When commit 0827e8af70f4 added auto-analyze support for partitioned
|    tables, it included code to obtain reltuples for the partitioned table
|    as a number of catalog accesses to read pg_class.reltuples for each
|    partition.  That's not only very inefficient, but also problematic
|    because autovacuum doesn't hold any locks on any of those tables -- and
|    doesn't want to.  Replace that code with a read of pg_class.reltuples
|    for the partitioned table, and make sure ANALYZE and TRUNCATE properly
|    maintain that value.
|    
|    I found no code that would be affected by the change of relpages from
|    zero to non-zero for partitioned tables, and no other code that should
|    be maintaining it, but if there is, hopefully it'll be an easy fix.

+       else if (onerel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+       {
+               /*
+                * Partitioned tables don't have storage, so we don't set any fields in
+                * their pg_class entries except for relpages, which is necessary for
+                * auto-analyze to work properly.
+                */
+               vac_update_relstats(onerel, -1, totalrows,
+                                                       0, false, InvalidTransactionId,
+                                                       InvalidMultiXactId,
+                                                       in_outer_xact);
+       }

This refers to "relpages", but I think it means "reltuples".

src/include/commands/vacuum.h:extern void vac_update_relstats(Relation relation,
src/include/commands/vacuum.h-                                BlockNumber num_pages,
src/include/commands/vacuum.h-                                double num_tuples,
src/include/commands/vacuum.h-                                BlockNumber num_all_visible_pages,

I'm adding it for the next round of "v14docs" patch if you don't want to make a
separate commit for that.

-- 
Justin

Re: pgsql: autovacuum: handle analyze for partitioned tables

Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-04-21T22:40:19Z

On 2021-Apr-08, Tom Lane wrote:

> BTW, another thing that looks like a race condition is the
> extract_autovac_opts() call that is done a little bit earlier,
> also without lock.  I think this is actually safe, but it's ONLY
> safe because we resisted the calls by certain people to add a
> toast table to pg_class.  Otherwise, fetching reloptions could
> have involved a toast pointer dereference, and it would then be
> racy whether the toasted data was still there.  As-is, even if
> the pg_class row we're looking at has been deleted, we can safely
> disassemble its reloptions.  I think this matter is deserving
> of a comment at least.

True.  I added a comment there.

Thanks,

-- 
Álvaro Herrera       Valdivia, Chile
"I am amazed at [the pgsql-sql] mailing list for the wonderful support, and
lack of hesitasion in answering a lost soul's question, I just wished the rest
of the mailing list could be like this."                               (Fotis)
               (http://archives.postgresql.org/pgsql-sql/2006-06/msg00265.php)

Re: pgsql: autovacuum: handle analyze for partitioned tables

Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-04-21T23:06:49Z

On 2021-Apr-09, Robert Haas wrote:

> Does this need to worry about new partitions getting attached to a
> partitioned table, or old ones getting detached? (Maybe it does
> already, not sure.)

I was pinged because this is listed as an open item.  I don't think it
is one.  Handling ATTACH/DETACH/DROP is important for overall
consistency, of course, so we should do it eventually, but the fact that
autovacuum runs analyze *at all* for partitioned tables is an enormous
step forward from it not doing so.  I think we should treat ATTACH/
DETACH/DROP handling as a further feature to be added in a future
release, not an open item to be fixed in the current one.

Now, if somebody sees a very trivial way to handle it, let's discuss it,
but *I* don't see it.

-- 
Álvaro Herrera       Valdivia, Chile
"You're _really_ hosed if the person doing the hiring doesn't understand
relational systems: you end up with a whole raft of programmers, none of
whom has had a Date with the clue stick."              (Andrew Sullivan)

Re: pgsql: autovacuum: handle analyze for partitioned tables

yuzuko <yuzukohosoya@gmail.com> — 2021-04-21T23:30:52Z

Hi,

Thank you for discussing this item.

> I think we should treat ATTACH/
> DETACH/DROP handling as a further feature to be added in a future
> release, not an open item to be fixed in the current one.
>
I agree with your opinion.

> Now, if somebody sees a very trivial way to handle it, let's discuss it,
> but *I* don't see it.
>
I started thinking about the way to handle ATTACH/DETACH/DROP,
but I haven't created patches.  If no one has done it yet, I'll keep working.

--
Best regards,
Yuzuko Hosoya
NTT Open Source Software Center

Re: pgsql: autovacuum: handle analyze for partitioned tables

Justin Pryzby <pryzby@telsasoft.com> — 2021-04-22T17:43:46Z

On Wed, Apr 21, 2021 at 07:06:49PM -0400, Alvaro Herrera wrote:
> On 2021-Apr-09, Robert Haas wrote:
> > Does this need to worry about new partitions getting attached to a
> > partitioned table, or old ones getting detached? (Maybe it does
> > already, not sure.)
> 
> I was pinged because this is listed as an open item.  I don't think it
> is one.  Handling ATTACH/DETACH/DROP is important for overall
> consistency, of course, so we should do it eventually, but the fact that
> autovacuum runs analyze *at all* for partitioned tables is an enormous
> step forward from it not doing so.  I think we should treat ATTACH/
> DETACH/DROP handling as a further feature to be added in a future
> release, not an open item to be fixed in the current one.

I think this is okay, with the caveat that we'd be changing the behavior
(again) in a future release, rather than doing it all in v14.

Maybe the behavior should be documented, though.  Actually, I thought the
pre-existing (non)behavior of autoanalyze would've been documented, and we'd
now update that.  All I can find is this:

https://www.postgresql.org/docs/current/sql-analyze.html
|The autovacuum daemon, however, will only consider inserts or updates on the
|parent table itself when deciding whether to trigger an automatic analyze for
|that table

I think that should probably have been written down somewhere other than for
the manual ANALYZE command, but in any case it seems to be outdated now.

-- 
Justin

Re: pgsql: autovacuum: handle analyze for partitioned tables

Justin Pryzby <pryzby@telsasoft.com> — 2021-04-23T18:01:52Z

On Thu, Apr 22, 2021 at 12:43:46PM -0500, Justin Pryzby wrote:
> Maybe the behavior should be documented, though.  Actually, I thought the
> pre-existing (non)behavior of autoanalyze would've been documented, and we'd
> now update that.  All I can find is this:
> 
> https://www.postgresql.org/docs/current/sql-analyze.html
> |The autovacuum daemon, however, will only consider inserts or updates on the
> |parent table itself when deciding whether to trigger an automatic analyze for
> |that table
> 
> I think that should probably have been written down somewhere other than for
> the manual ANALYZE command, but in any case it seems to be outdated now.

Starting with this

Re: pgsql: autovacuum: handle analyze for partitioned tables

Michael Paquier <michael@paquier.xyz> — 2021-05-11T08:08:43Z

On Wed, Apr 21, 2021 at 07:06:49PM -0400, Alvaro Herrera wrote:
> On 2021-Apr-09, Robert Haas wrote:
>> Does this need to worry about new partitions getting attached to a
>> partitioned table, or old ones getting detached? (Maybe it does
>> already, not sure.)
> 
> I was pinged because this is listed as an open item.  I don't think it
> is one.  Handling ATTACH/DETACH/DROP is important for overall
> consistency, of course, so we should do it eventually, but the fact that
> autovacuum runs analyze *at all* for partitioned tables is an enormous
> step forward from it not doing so.  I think we should treat ATTACH/
> DETACH/DROP handling as a further feature to be added in a future
> release, not an open item to be fixed in the current one.

Yeah, I'd agree that this could be done as some future work so it
looks fine to move it to the section for "won't fix" items, but that
sounds rather tricky to me as there are dependencies across the
partitions.

Now, I don't think that we are completely done either, as one
documentation patch has been sent here:
https://www.postgresql.org/message-id/20210423180152.GA17270@telsasoft.com

Alvaro, could you look at that?
--
Michael

Re: pgsql: autovacuum: handle analyze for partitioned tables

Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-05-11T21:56:16Z

On 2021-Apr-23, Justin Pryzby wrote:

> On Thu, Apr 22, 2021 at 12:43:46PM -0500, Justin Pryzby wrote:
> > 
> > I think that should probably have been written down somewhere other than for
> > the manual ANALYZE command, but in any case it seems to be outdated now.
> 
> Starting with this 

Agreed, we need some more docs here.  I lightly edited yours and ended
up with this -- mostly I think partitioned tables should not be in the
same paragraph as legacy inheritance because the behavior is different
enough (partitioned tables are not analyzed twice).

I'll give a deeper look tomorrow to see if other places also need edits.

Thanks

-- 
Álvaro Herrera       Valdivia, Chile

Re: pgsql: autovacuum: handle analyze for partitioned tables

Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-05-13T21:33:33Z

New version, a bit more ambitious.  I think it's better to describe
behavior for partitioned tables ahead of inheritance.  Also, in the
ANALYZE reference page I split the topic in two: in one single paragraph
we now describe what happens with manual analyze for partitioned tables
and inheritance hierarchies; we describe the behavior of autovacuum in
one separate paragraph for each type of hierarchy, since the differences
are stark.

I noticed that difference while verifying the behavior that I was to
document.  If you look at ANALYZE VERBOSE output, it seems a bit
wasteful:

create table part (a int) partition by list (a);
create table part0 partition of part for values in (0);
create table part1 partition of part for values in (1);
create table part23 partition of part for values in (2, 3) partition by list (a);
create table part2 partition of part23 for values in (2);
create table part3 partition of part23 for values in (3);
insert into part select g%4 from generate_series(1, 50000000) g;

analyze verbose part;

INFO:  analyzing "public.part" inheritance tree
INFO:  "part1": scanned 7500 of 55310 pages, containing 1695000 live rows and 0 dead rows; 7500 rows in sample, 12500060 estimated total rows
INFO:  "part2": scanned 7500 of 55310 pages, containing 1695000 live rows and 0 dead rows; 7500 rows in sample, 12500060 estimated total rows
INFO:  "part3": scanned 7500 of 55310 pages, containing 1695000 live rows and 0 dead rows; 7500 rows in sample, 12500060 estimated total rows
INFO:  "part4": scanned 7500 of 55310 pages, containing 1695000 live rows and 0 dead rows; 7500 rows in sample, 12500060 estimated total rows
INFO:  analyzing "public.part1"
INFO:  "part1": scanned 30000 of 55310 pages, containing 6779940 live rows and 0 dead rows; 30000 rows in sample, 12499949 estimated total rows
INFO:  analyzing "public.part2"
INFO:  "part2": scanned 30000 of 55310 pages, containing 6779940 live rows and 0 dead rows; 30000 rows in sample, 12499949 estimated total rows
INFO:  analyzing "public.part34" inheritance tree
INFO:  "part3": scanned 15000 of 55310 pages, containing 3390000 live rows and 0 dead rows; 15000 rows in sample, 12500060 estimated total rows
INFO:  "part4": scanned 15000 of 55310 pages, containing 3389940 live rows and 0 dead rows; 15000 rows in sample, 12499839 estimated total rows
INFO:  analyzing "public.part3"
INFO:  "part3": scanned 30000 of 55310 pages, containing 6780000 live rows and 0 dead rows; 30000 rows in sample, 12500060 estimated total rows
INFO:  analyzing "public.part4"
INFO:  "part4": scanned 30000 of 55310 pages, containing 6780000 live rows and 0 dead rows; 30000 rows in sample, 12500060 estimated total rows
ANALYZE

-- 
Álvaro Herrera       Valdivia, Chile
"The eagle never lost so much time, as
when he submitted to learn of the crow." (William Blake)

Re: pgsql: autovacuum: handle analyze for partitioned tables

Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-05-13T23:02:23Z

With English fixes from Bruce.

I think the note about autovacuum in the reference page for ANALYZE is a
bit out of place, but not enough to make me move the whole paragraph
elsewhere.  Maybe we should try to do that sometime.

-- 
Álvaro Herrera       Valdivia, Chile
Officer Krupke, what are we to do?
Gee, officer Krupke, Krup you! (West Side Story, "Gee, Officer Krupke")

Re: pgsql: autovacuum: handle analyze for partitioned tables

Justin Pryzby <pryzby@telsasoft.com> — 2021-05-13T23:25:17Z

On Thu, May 13, 2021 at 05:33:33PM -0400, Alvaro Herrera wrote:
> +++ b/doc/src/sgml/maintenance.sgml
> @@ -817,6 +817,11 @@ analyze threshold = analyze base threshold + analyze scale factor * number of tu
>  </programlisting>
>      is compared to the total number of tuples inserted, updated, or deleted
>      since the last <command>ANALYZE</command>.
> +    For partitioned tables, inserts and updates on partitions are counted
> +    towards this threshold; however partition meta-operations such as
> +    attachment, detachment or drop are not, so running a manual
> +    <command>ANALYZE</command> is recommended if the partition added or
> +    removed contains a statistically significant volume of data.

I suggest: "Inserts, updates and deletes on partitions of a partitioned table
are counted towards this threshold; however DDL operations such as ATTACH,
DETACH and DROP are not, ...

> +   and in addition it will analyze each individual partition separately.

remove "and" and say in addition COMMA
Or:
"it will also recursive into each partition and update its statistics."

> +   By constrast, if the table being analyzed has inheritance children,
> +   <command>ANALYZE</command> will gather statistics for that table twice:
> +   once on the rows of the parent table only, and a second time on the
> +   rows of the parent table with all of its children.  This second set of
> +   statistics is needed when planning queries that traverse the entire
> +   inheritance tree.  The children tables are not individually analyzed
> +   in this case.

say "The child tables themselves.."

> +  <para>
> +   For tables with inheritance children, the autovacuum daemon only
> +   counts inserts and deletes in the parent table itself when deciding
> +   whether to trigger an automatic analyze for that table.  If that table
> +   is rarely inserted into or updated, the inheritance statistics will
> +   not be up to date unless you run <command>ANALYZE</command> manually.
> +  </para>

This should be emphasized:
Tuples changed in inheritence children do not count towards analyze on the
parent table.  If the parent table is empty or rarely changed, it may never 
be processed by autovacuum.  It's necesary to periodically run an manual
ANALYZE to keep the statistics of the table hierarchy up to date.

I don't know why it says "inserted or updated" but doesn't say "or deleted" -
that seems like a back-patchable fix.

> +++ b/doc/src/sgml/ref/pg_restore.sgml
> @@ -922,8 +922,10 @@ CREATE DATABASE foo WITH TEMPLATE template0;
>  
>    <para>
>     Once restored, it is wise to run <command>ANALYZE</command> on each
> -   restored table so the optimizer has useful statistics; see
> -   <xref linkend="vacuum-for-statistics"/> and
> +   restored table so the optimizer has useful statistics.
> +   If the table is a partition or an inheritance child, it may also be useful
> +   to analyze the parent table.
> +   See <xref linkend="vacuum-for-statistics"/> and
>     <xref linkend="autovacuum"/> for more information.

maybe say: "analyze the parent to update statistics for the table hierarchy".

Re: pgsql: autovacuum: handle analyze for partitioned tables

Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-05-14T17:13:12Z

Thanks for these corrections -- I applied them and a few minor changes
from myself, and pushed.  Another set of eyes over the result would be
most welcome.

I hope we can close this now :-)

-- 
Álvaro Herrera                            39°49'30"S 73°17'W
"Those who use electric razors are infidels destined to burn in hell while
we drink from rivers of beer, download free vids and mingle with naked
well shaved babes." (http://slashdot.org/comments.pl?sid=44793&cid=4647152)