Re: Table-level log_autovacuum_min_duration

On Fri, Apr 3, 2015 at 5:57 AM, Alvaro Herrera wrote:
> 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.

I did consider the case of wraparound but came to the conclusion that
bailing out as fast as possible was the idea. But well, I guess that
we could play it safe and not add this check after all. The main
use-case of this change is now to be able to do re-balancing of the
cost parameters. We could still decide later if a worker could bail
out earlier or not, depending on what.

> 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.

This sounds like a good idea, and this refactoring would meritate a
different patch and a different thread. I guess that it should be a
new section in Server Configuration then, named "Relation Options",
with two different subsections for index options and table options.

> 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.

Check. I still think that it is useful in this case though... But removed.

> I think this could use some wordsmithing; I didn't like listing the
> parameters explicitely, and Jaime Casanova suggested not using the terms
> "inherit" and "parent table" in this context.  So I came up with this:
>   Note that the TOAST table uses the parameter values defined for the main
>   table, for each parameter applicable to TOAST tables and for which no
>   value is set in the TOAST table itself.

Fine for me.
--
Michael