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