Re: fillfactor hides autovacuum parameters in 8.4.0

PostgreSQL version: 8.4.0
Operating system: all versions

If we set FILLFACTOR to a table, autovacuum parameters in postgresql.conf
will not affect to the table; default values are always used.

In 8.4.0, we create StdRdOptions if a relation has some fields in
pg_class.reloption. Then, unspecified fields are filled with the
default values. The values are always hard-coded default values and
don't reflect current GUC settings.

For example:
pg_class.reloptions = {fillfactor=70}
makes
StdRdOptions { fillfactor=70, vacuum_scale_factor=0.2, ... }
~~~~~~~~~~~~~~~~~~~~~~~
always default

To fix the bug, each field in StdRdOptions should have "not-specified" flag.
If not specified, autovacuum should use current GUC settings instead of
reloptions. Is it possible to change the default values of reloptions
to some magic number (-1 or so) ?

There might be another idea that we fill StdRdOptions with current GUC
settings instead of hard-coded default values. It looks cleaner, but
we need to treat dynamic default values and invalidate reloptions
if we reload settings.

How do we fix it?

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

Itagaki Takahiro escreveu:
> In 8.4.0, we create StdRdOptions if a relation has some fields in
> pg_class.reloption. Then, unspecified fields are filled with the
> default values. The values are always hard-coded default values and
> don't reflect current GUC settings.
>
Hey, how I couldn't notice that. :(

> To fix the bug, each field in StdRdOptions should have "not-specified" flag.
> If not specified, autovacuum should use current GUC settings instead of
> reloptions. Is it possible to change the default values of reloptions
> to some magic number (-1 or so) ?
>
BTW, we agreed that magic numbers is a hack in pg_autovacuum and we wouldn't
use it in reloptions [1].

> There might be another idea that we fill StdRdOptions with current GUC
> settings instead of hard-coded default values. It looks cleaner, but
> we need to treat dynamic default values and invalidate reloptions
> if we reload settings.
>
+1. I'm afraid we need to touch too much code for a fix. Let's do it for 8.5.
I'll try to come up with a patch.

A not-so-invasive code is to transform all AutoVacOpts elements in pointers
and to leave the default assignment to relation_needs_vacanalyze(). If nobody
steps up I'll give it a stab later.

[1] http://archives.postgresql.org/pgsql-hackers/2009-02/msg00303.php

--
Euler Taveira de Oliveira
http://www.timbira.com/

Itagaki Takahiro wrote:
> PostgreSQL version: 8.4.0
> Operating system: all versions
>
> If we set FILLFACTOR to a table, autovacuum parameters in postgresql.conf
> will not affect to the table; default values are always used.
>
> In 8.4.0, we create StdRdOptions if a relation has some fields in
> pg_class.reloption. Then, unspecified fields are filled with the
> default values. The values are always hard-coded default values and
> don't reflect current GUC settings.

Hmm, I'm fairly sure the code had this right originally ... maybe it got
drowned in some of the infinite reworking of that patch and I neglected
to test it. I'll have a look tomorrow.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Itagaki Takahiro wrote:

> To fix the bug, each field in StdRdOptions should have "not-specified" flag.
> If not specified, autovacuum should use current GUC settings instead of
> reloptions. Is it possible to change the default values of reloptions
> to some magic number (-1 or so) ?

Ah. After checking the code I remember that this was right at some
point, but it was lost when the parsing step was made to use a table
(fillRelOptions). This behavior could be restored by having the fill
routine have a callback of some sort; we could use that to set a boolean
in StdRdOptions that indicated whether they are set in reloptions or are
default values.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote:

> we could use that to set a boolean
> in StdRdOptions that indicated whether they are set in reloptions or are
> default values.

Do you mean we will have a boolean field *for each* field in StdRdOptions?
We should remember whether a field was specified or not independntly.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

Here is a patch to fix a bug in handling default values in reloptions.
This fix should be applied to HEAD and 8.4.0.

I used 'magic number -1' to propagate "not-specified" information to
autovacuum process. It might look strange because the default value is
out of range of the reloption, but I think it has less impact to the
codes comapred with other solutions (dynamic default values etc.).

> To fix the bug, each field in StdRdOptions should have "not-specified" flag.
> If not specified, autovacuum should use current GUC settings instead of
> reloptions. Is it possible to change the default values of reloptions
> to some magic number (-1 or so) ?

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

Itagaki Takahiro wrote:
>
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote:
>
> > we could use that to set a boolean
> > in StdRdOptions that indicated whether they are set in reloptions or are
> > default values.
>
> Do you mean we will have a boolean field *for each* field in StdRdOptions?

No, I was thinking in a single value for all of autovac options ...

> We should remember whether a field was specified or not independntly.

I'm not sure what you are suggesting ...?

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote:

> > We should remember whether a field was specified or not independntly.
> I'm not sure what you are suggesting ...?

Please imagine that:

=# SET autovacuum_vacuum_scale_factor = 0.05;
=# ALTER TABLE tbl SET (autovacuum_vacuum_threshold = 10);

AutoVacOpts.vacuum_threshold should be 10 (comes from reloptions),
but AutoVacOpts.vacuum_scale_factor should be <DEFAULT> (comes from GUC).

If we use boolean flags, we need booleans for each fields in AutoVacOpts.
(vacuum_threshold_is_default, vacuum_scale_factor_is_default, ...)

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

Itagaki Takahiro wrote:
>
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote:
>
> > > We should remember whether a field was specified or not independntly.
> > I'm not sure what you are suggesting ...?
>
> Please imagine that:
>
> =# SET autovacuum_vacuum_scale_factor = 0.05;
> =# ALTER TABLE tbl SET (autovacuum_vacuum_threshold = 10);

Uh, but this doesn't work because you can't SET inside a session and
have autovacuum read it. Only the values that you change with ALTER
TABLE can be read by autovacuum; the rest of the values come from
postgresql.conf.

But I'm very dizzy today so perhaps I'm missing something obvious.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Itagaki Takahiro wrote:
>
> Here is a patch to fix a bug in handling default values in reloptions.
> This fix should be applied to HEAD and 8.4.0.
>
> I used 'magic number -1' to propagate "not-specified" information to
> autovacuum process. It might look strange because the default value is
> out of range of the reloption, but I think it has less impact to the
> codes comapred with other solutions (dynamic default values etc.).

I realized that any other solution here is going to be more complex and
thus less appropriate for backpatch. I still don't like this very much
because it doesn't seem to offer enough flexibility to user-specified
reloptions; but any patch we come up with here is going to be applicable
to CVS HEAD.

So I'm going to apply your patch to both 8.4 and HEAD; we can always
improve it later, I guess.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote:

> So I'm going to apply your patch to both 8.4 and HEAD; we can always
> improve it later, I guess.

Thank you for your applying.
I think the fix is ugly, too. We need to introduce cleaner solution for 8.5.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center