Thread: Re: Autovacuum integration

Hackers,

Here is a second attempt at autovacuum integration.  Please have a look
at it.  Note that this patch automatically creates three new files:

src/backend/postmaster/autovacuum.c
src/include/catalog/pg_autovacuum.h
src/include/postmaster/autovacuum.h


There are the following changes since the previous version:

- Xid wraparound is taken care of, with a database-wide VACUUM when the
  limit is too close.

- Manually executing VACUUM or ANALYZE now updates the statistics for
  autovacuum.

- EXEC_BACKEND case was relieved of a few bugs -- it now actually works.

- elog(ERROR) is now handled more gracefully; the error is emitted,
  instead of a SEGV happening.  The end result is the same, however:
  a single error takes the process down, and the database will be
  vacuumed again only after all other databases are processed.

- Adapted to the new cwd convention.

- My email address is changed, so that I won't be posting the patch
  several times hopefully :-)


Note that I didn't make the autovacuum daemon more verbose.  Running
postmaster -d2 (or server_log_messages or whatever the option is called,
to DEBUG2 or higher) shows some messages which are, I hope, what Matthew
was expecting.  (It this is not the case please let me know.)

--
Alvaro Herrera (<alvherre[a]alvh.no-ip.org>)
"El sabio habla porque tiene algo que decir;
el tonto, porque tiene que decir algo" (Platon).

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> Here is a second attempt at autovacuum integration.

A few comments:

* I strongly disagree with keeping updatable state in a catalog.  In the
first place, that will cause autovac itself to create vacuumable trash.
In the second place, that means you can't modify the internal state kept
by autovacuum without forcing initdb; which is not a good situation,
considering how much change this code is likely to go through.
I think all the updatable state should be kept in the pgstats file.
(Memo to self: let's add a version number header to the pgstats file,
so that we can change its format without requiring an initdb to do so.)
pg_autovacuum should only contain user-settable parameters --- which is
still putting us at nontrivial risk for initdb constraints, but it's way
better than internal state.  If you do that, then pg_autovacuum need
only contain entries for tables that have been given non-default
parameter settings (ie, those for which the user wants to override
global settings given in postgresql.conf).

* I'm pretty dubious about adding a syscache for pg_autovacuum.
Where exactly are the repeated fetches going to come from to justify
caching it?

* The signal processing needs re-work; you can't just randomly assign
signal numbers, you need to keep these things largely consistent with
the other subprocesses.  In particular, if this thing is going to be
running transactions then it MUST honor SIGALRM (eg, for deadlock
timeout checks) and SIGUSR1 (sinval catchup interrupt), and I don't
think you get to ignore SIGTERM either (may get this from init).
I think it's a pretty bad idea to use SIGUSR2 for something other than
what regular backends use it for, too.  (Consider the prospect that for
some reason your PID occurs in pg_listener.)  It'd be a good idea to
honor SIGINT with the normal meaning (query cancel, ie, abort
transaction, which in this context would also imply process shutdown)
and use that to shut down the daemon at postmaster stop.  In short,
the signal handling had better look a whole lot more like a normal
backend's.

* Speaking of shutdown, you can't stop the bgwriter until you know
that the autovac daemon is dead.  In this respect too, it's much more
like a backend than like any of the other support processes.  Signal it
when you signal the backends, and don't advance to the bgwriter kill
phase until autovac and all the backends are known gone.

* I see you have an autovac_init function to "annoy the user", but
shouldn't this be checked every time we are about to spawn an autovac
process?

* I don't see any special checks for shared catalogs, which means they
are probably going to be over-vacuumed; or possibly under-vacuumed if
you fail to track the update stats for them in a single place rather
than in each database.  It'd probably be a good idea to nominate just
one database to be the place from which shared catalogs are vacuumed;
or treat them as a completely separate special case.

* I have no objection to adding extra entry points to vacuum.c to
simplify the calls to it.

* With respect to comments like:

+         /*
+          * We just skip a table not found on the stat hash.  This is
+          * because whatever we do here, there won't be good statistics
+          * next time around, so we would do this same action again.  It is
+          * tempting to issue an ANALYZE, but it won't work because ANALYZE
+          * doesn't update the stat system.
+          */

ISTM the entire point of "autovac integration" is to make sure the rest
of the system does what's needed to support autovac.  If ANALYZE needs
to send something to the stats system, make it do so.  (ISTM it'd be
reasonable for ANALYZE to make and report an estimate of the dead tuple
count, which after all is what you are hoping all the rest of the stats
machinery will derive for you...)

            regards, tom lane

On Fri, Jul 08, 2005 at 03:56:25PM -0400, Tom Lane wrote:
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > Here is a second attempt at autovacuum integration.
>
> A few comments:

Excellent, thanks.  I'll be working on fixing all these.

> * I strongly disagree with keeping updatable state in a catalog.  In the
> first place, that will cause autovac itself to create vacuumable trash.

Yes, I thought about that too.  I think the argument that convinced me
to do otherwise was the possibility of messages being lost in the path
to stat collector.  And I thought that it would be possible to change
tuples in-place, like vacuum does for pg_class.  I don't have a problem
with changing it though.

The initdb-forcing point is a good one, didn't think of it.

> pg_autovacuum should only contain user-settable parameters --- which is
> still putting us at nontrivial risk for initdb constraints, but it's way
> better than internal state.  If you do that, then pg_autovacuum need
> only contain entries for tables that have been given non-default
> parameter settings.

Ok.  Do you think it's worth the trouble to have ALTER TABLE commands to
change autovac parameters?

> * The signal processing needs re-work; you can't just randomly assign
> signal numbers, you need to keep these things largely consistent with
> the other subprocesses.

I think I copied this from pgarch.c, which I can see was a really bad
idea.  Will fix.

> * I see you have an autovac_init function to "annoy the user", but
> shouldn't this be checked every time we are about to spawn an autovac
> process?

You argued exactly the other way around to Matthew, before 8.0 freeze.
Personally I don't care either way.


> * I don't see any special checks for shared catalogs, which means they
> are probably going to be over-vacuumed; or possibly under-vacuumed if
> you fail to track the update stats for them in a single place rather
> than in each database.  It'd probably be a good idea to nominate just
> one database to be the place from which shared catalogs are vacuumed;
> or treat them as a completely separate special case.

Yeah.  The problem with this is that any particular database could be
absent from the stat hash.  Personally I think we should make the stat
system aware of all databases, whether there has been activity in them
or not.  That'd make this problem moot, and we could skip those tables
in any databases except template1.


Anything I omitted I agree with, so I'll fix things up accordingly.

--
Alvaro Herrera (<alvherre[a]alvh.no-ip.org>)
"The Gord often wonders why people threaten never to come back after they've
been told never to return" (www.actsofgord.com)

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On Fri, Jul 08, 2005 at 03:56:25PM -0400, Tom Lane wrote:
>> pg_autovacuum should only contain user-settable parameters ---

> Ok.  Do you think it's worth the trouble to have ALTER TABLE commands to
> change autovac parameters?

I think we'll want them eventually, but I don't mind if 8.1 ships
without 'em.

>> * I see you have an autovac_init function to "annoy the user", but
>> shouldn't this be checked every time we are about to spawn an autovac
>> process?

> You argued exactly the other way around to Matthew, before 8.0 freeze.

Did I?  I've forgotten the reasoning if so.  But it's a minor point.

> Yeah.  The problem with this is that any particular database could be
> absent from the stat hash.  Personally I think we should make the stat
> system aware of all databases, whether there has been activity in them
> or not.  That'd make this problem moot, and we could skip those tables
> in any databases except template1.

Hm.  There's nothing wrong with having CREATE DATABASE send a message to
pgstats, but that could get lost anyway.  A probably more significant
point is that either postgres or template1 is subject to being dropped;
so hardwiring a single database to do it from is likely not gonna work.

It'd be okay to *consider* vacuuming the shared relations on every
cycle, so long as the stats were managed correctly (ie, only one set of
stats kept).  This is probably easier to do in the context of the
stats-in-pgstats idea than before.  We could have pgstats deliberately
create an entry for "database zero", and put the table stats for shared
relations there.  The only real issue is that we have no place to keep
any nondefault parameters for these catalogs (no, I don't wish to invent
pg_shautovacuum to fix that ;-)) so they'd always be autovacuumed with
the global default parameters.  This seems acceptable to me.

            regards, tom lane

Tom Lane wrote:

>Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
>
>
>>Ok.  Do you think it's worth the trouble to have ALTER TABLE commands to
>>change autovac parameters?
>>
>>
>
>I think we'll want them eventually, but I don't mind if 8.1 ships
>without 'em.
>
>

It might be good to ship 8.1 without them since autovacuum might change
a fair amount between 8.1 and 8.2 (FSM integration, better formula for
vacuum decisions etc...)


>>Yeah.  The problem with this is that any particular database could be
>>absent from the stat hash.  Personally I think we should make the stat
>>system aware of all databases, whether there has been activity in them
>>or not.  That'd make this problem moot, and we could skip those tables
>>in any databases except template1.
>>
>>
>
>Hm.  There's nothing wrong with having CREATE DATABASE send a message to
>pgstats, but that could get lost anyway.  A probably more significant
>point is that either postgres or template1 is subject to being dropped;
>so hardwiring a single database to do it from is likely not gonna work.
>
>It'd be okay to *consider* vacuuming the shared relations on every
>cycle, so long as the stats were managed correctly (ie, only one set of
>stats kept).  This is probably easier to do in the context of the
>stats-in-pgstats idea than before.  We could have pgstats deliberately
>create an entry for "database zero", and put the table stats for shared
>relations there.  The only real issue is that we have no place to keep
>any nondefault parameters for these catalogs (no, I don't wish to invent
>pg_shautovacuum to fix that ;-)) so they'd always be autovacuumed with
>the global default parameters.  This seems acceptable to me.
>

If memory serves from development discussion of the contrib version,
isn't there an issue that while a shared relation need only be vacuumed
in one database in order to reclaim space etc, the analyze statistics
need to be updated in each database.  Does that ring a bell or am I way
off base here?

"Matthew T. O'Connor" <matthew@zeut.net> writes:
> Tom Lane wrote:
>> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
>>> Ok.  Do you think it's worth the trouble to have ALTER TABLE commands to
>>> change autovac parameters?
>>
>> I think we'll want them eventually, but I don't mind if 8.1 ships
>> without 'em.
>>
> It might be good to ship 8.1 without them since autovacuum might change
> a fair amount between 8.1 and 8.2 (FSM integration, better formula for
> vacuum decisions etc...)

Yeah, I was thinking the same thing: the more support there is, the
harder it will be to change the parameter set.  Leaving it as a catalog
only will send a suitable message that this is still a prototype ...

> If memory serves from development discussion of the contrib version,
> isn't there an issue that while a shared relation need only be vacuumed
> in one database in order to reclaim space etc, the analyze statistics
> need to be updated in each database.  Does that ring a bell or am I way
> off base here?

That's a fair point, but I'm not sure how much we care about maintaining
stats for the shared relations.  These days the planner should get it
reasonably right about the relation size anyway, and the availability of
pg_statistic data is only a secondary concern; especially since we have
unique indexes on most of the columns people might search on, and the
presence of such an index also clues the planner.

            regards, tom lane

On Fri, Jul 08, 2005 at 03:56:25PM -0400, Tom Lane wrote:
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > Here is a second attempt at autovacuum integration.
>
> A few comments:

Ok, here is an updated patch.  Hopefully I have covered most your
more important observations.  Particularly, I changed the shutdown
sequence per your comments, and the pg_autovacuum tuple is optional.

Additional comments:

> * I see you have an autovac_init function to "annoy the user", but
> shouldn't this be checked every time we are about to spawn an autovac
> process?

I didn't do anything about this (i.e. it only happens once).  Note that
if we annoy the user because of this, the autovacuum process is
disabled "forever."

> * I don't see any special checks for shared catalogs, which means they
> are probably going to be over-vacuumed; or possibly under-vacuumed if
> you fail to track the update stats for them in a single place rather
> than in each database.

I'm still not doing anything special about shared relations.  I think it
would be easy to treat them in a special way.

> * I have no objection to adding extra entry points to vacuum.c to
> simplify the calls to it.

I didn't do it, because it uglified the code.  Rather, I added a relid
member to VacuumStmt.

> If ANALYZE needs to send something to the stats system, make it do
> so.

It does, as does VACUUM.  I still think we should do something special
on TRUNCATE, maybe send a special message.


Note that I keep track of dead tuples directly in the stats for each
table, rather than keeping track of "last vacuum tuples", which was a
strange concept anyway.  It came out being much simpler this way.  The
only consideration is that it makes the vacuum case different from
analyze, but I don't see that as a problem.

Also, there are tables for which analyze refuses to run.  I'm not sure
what to do about them.  The problem is that since ANALYZE doesn't run to
completion, it doesn't emit the stat message, so we try to analyze it
the next time around.  I considered sending special messages to the
stats, telling not to analyze the table in the future (vacuum still
works as expected).  However I don't see how would we re-enable the
auto-analyze feature in case something happens to the table.  There are
two cases: pg_statistics is one, and the other is tables that don't have
any analyzable columns (There is at least one table of this kind in the
regression test, comprising one "box" column.)  This may turn out not to
be a problem, since ANALYZE will return very quickly in this case, but
it annoys me anyway.

Finally: I didn't do anything special about TOAST tables yet.  I think
this is a separate problem.

--
Alvaro Herrera (<alvherre[a]alvh.no-ip.org>)
Thou shalt study thy libraries and strive not to reinvent them without
cause, that thy code may be short and readable and thy days pleasant
and productive. (7th Commandment for C Programmers)