pgsql: autovacuum: handle analyze for partitioned tables

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&amp;dt=2021-04-08%2005%3A50%3A08

regards, tom lane

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&amp;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)

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&amp;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

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

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

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)

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

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

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

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)

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

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)

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

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

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)

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

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&amp;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

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&amp;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

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&amp;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