Re: Table-level log_autovacuum_min_duration

Michael Paquier wrote:

> So, attached are two patches:
> - 0001 enables SIGHUP tracking in do_autovacuum(), which is checked
> before processing one table. I reused avl_sighup_handler for the
> worker, renaming it av_sighup_handler..
> - 0002 is the patch to add log_autovacuum_min_duration as a reloption.
> There is nothing really changed, the value of
> log_autovacuum_min_duration being picked up at startup with
> table_recheck_autovac() is used until the end for one table.

Thanks.

You added this in the worker loop processing each table:

/*
* Check for config changes before processing each collected table.
*/
if (got_SIGHUP)
{
got_SIGHUP = false;
ProcessConfigFile(PGC_SIGHUP);

/* shutdown requested in config file? */
if (!AutoVacuumingActive())
break;
}

I think bailing out if autovac is disabled is a good idea; for instance,
if this is a for-wraparound vacuum, we should keep running. My first
reaction is that this part of the test ought to be moved down a
screenful or so, until we've ran the recheck step and we have the
for-wraparound flag in hand; only bail out if that one is not set. But
on the other hand, maybe the worker will process some tables that are
not for-wraparound in between some other tables that are, so that might
turn out to be a bad idea as well. (Though I'm not 100% that it works
that way; consider commit f51ead09df for instance.) Maybe the test to
use for this is something along the lines of "if autovacuum was enabled
before and is no longer enabled, bail out, otherwise keep running".
This implies that we need to keep track of whether autovac was enabled
at the start of the worker run.

Another thing, but not directly related to this patch: while looking at
the doc changes, I noticed that we don't have index entries for the
reloptions we have; for instance, the word "fillfactor" doesn't appear
in the bookindex.html page at all. Having these things all crammed in
the CREATE TABLE page seems somewhat poor. I think we could improve on
this by having a listing of settable parameters in a separate section,
and have CREATE TABLE, ALTER TABLE, and the Server Configuration chapter
link to that; we could also have index entries for these items.

Of course, the simpler fix is to just add a few <indexterm> tags.

As a note, there is no point to "Assert(foo != NULL)" tests when foo is
later dereferenced in the same routine: the crash is going to happen
anyway at the dereference, so it's just a LOC uselessly wasted.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services