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]http://archives.postgresql.org/pgsql-hackers/2009-02/msg00303.php.
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 versionsIf 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@commandprompt.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
Attachments:
fix-relopts-20090820.patchapplication/octet-stream; name=fix-relopts-20090820.patchDownload
diff -cprN head/src/backend/access/common/reloptions.c fix-relopts/src/backend/access/common/reloptions.c
*** head/src/backend/access/common/reloptions.c 2009-06-11 23:48:53.000000000 +0900
--- fix-relopts/src/backend/access/common/reloptions.c 2009-08-20 09:40:09.900157029 +0900
*************** static relopt_int intRelOpts[] =
*** 108,114 ****
"Minimum number of tuple updates or deletes prior to vacuum",
RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
},
! 50, 0, INT_MAX
},
{
{
--- 108,114 ----
"Minimum number of tuple updates or deletes prior to vacuum",
RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
},
! -1, 0, INT_MAX
},
{
{
*************** static relopt_int intRelOpts[] =
*** 116,122 ****
"Minimum number of tuple inserts, updates or deletes prior to analyze",
RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
},
! 50, 0, INT_MAX
},
{
{
--- 116,122 ----
"Minimum number of tuple inserts, updates or deletes prior to analyze",
RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
},
! -1, 0, INT_MAX
},
{
{
*************** static relopt_int intRelOpts[] =
*** 124,130 ****
"Vacuum cost delay in milliseconds, for autovacuum",
RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
},
! 20, 0, 100
},
{
{
--- 124,130 ----
"Vacuum cost delay in milliseconds, for autovacuum",
RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
},
! -1, 0, 100
},
{
{
*************** static relopt_int intRelOpts[] =
*** 132,138 ****
"Vacuum cost amount available before napping, for autovacuum",
RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
},
! 200, 1, 10000
},
{
{
--- 132,138 ----
"Vacuum cost amount available before napping, for autovacuum",
RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
},
! -1, 1, 10000
},
{
{
*************** static relopt_int intRelOpts[] =
*** 140,146 ****
"Minimum age at which VACUUM should freeze a table row, for autovacuum",
RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
},
! 100000000, 0, 1000000000
},
{
{
--- 140,146 ----
"Minimum age at which VACUUM should freeze a table row, for autovacuum",
RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
},
! -1, 0, 1000000000
},
{
{
*************** static relopt_int intRelOpts[] =
*** 148,161 ****
"Age at which to autovacuum a table to prevent transaction ID wraparound",
RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
},
! 200000000, 100000000, 2000000000
},
{
{
"autovacuum_freeze_table_age",
"Age at which VACUUM should perform a full table sweep to replace old Xid values with FrozenXID",
RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
! }, 150000000, 0, 2000000000
},
/* list terminator */
{{NULL}}
--- 148,161 ----
"Age at which to autovacuum a table to prevent transaction ID wraparound",
RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
},
! -1, 100000000, 2000000000
},
{
{
"autovacuum_freeze_table_age",
"Age at which VACUUM should perform a full table sweep to replace old Xid values with FrozenXID",
RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
! }, -1, 0, 2000000000
},
/* list terminator */
{{NULL}}
*************** static relopt_real realRelOpts[] =
*** 169,175 ****
"Number of tuple updates or deletes prior to vacuum as a fraction of reltuples",
RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
},
! 0.2, 0.0, 100.0
},
{
{
--- 169,175 ----
"Number of tuple updates or deletes prior to vacuum as a fraction of reltuples",
RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
},
! -1, 0.0, 100.0
},
{
{
*************** static relopt_real realRelOpts[] =
*** 177,183 ****
"Number of tuple inserts, updates or deletes prior to analyze as a fraction of reltuples",
RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
},
! 0.1, 0.0, 100.0
},
/* list terminator */
{{NULL}}
--- 177,183 ----
"Number of tuple inserts, updates or deletes prior to analyze as a fraction of reltuples",
RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
},
! -1, 0.0, 100.0
},
/* list terminator */
{{NULL}}
diff -cprN head/src/backend/postmaster/autovacuum.c fix-relopts/src/backend/postmaster/autovacuum.c
*** head/src/backend/postmaster/autovacuum.c 2009-08-13 05:53:30.000000000 +0900
--- fix-relopts/src/backend/postmaster/autovacuum.c 2009-08-20 10:03:51.979257913 +0900
*************** table_recheck_autovac(Oid relid, HTAB *t
*** 2448,2472 ****
* toast table, try the main table too. Otherwise use the GUC
* defaults, autovacuum's own first and plain vacuum second.
*/
! if (avopts)
! {
! vac_cost_delay = avopts->vacuum_cost_delay;
! vac_cost_limit = avopts->vacuum_cost_limit;
! freeze_min_age = avopts->freeze_min_age;
! freeze_table_age = avopts->freeze_table_age;
! }
! else
! {
! /* -1 in autovac setting means use plain vacuum_cost_delay */
! vac_cost_delay = autovacuum_vac_cost_delay >= 0 ?
! autovacuum_vac_cost_delay : VacuumCostDelay;
! /* 0 or -1 in autovac setting means use plain vacuum_cost_limit */
! vac_cost_limit = autovacuum_vac_cost_limit > 0 ?
! autovacuum_vac_cost_limit : VacuumCostLimit;
! /* these do not have autovacuum-specific settings */
! freeze_min_age = default_freeze_min_age;
! freeze_table_age = default_freeze_table_age;
! }
tab = palloc(sizeof(autovac_table));
tab->at_relid = relid;
--- 2448,2476 ----
* toast table, try the main table too. Otherwise use the GUC
* defaults, autovacuum's own first and plain vacuum second.
*/
!
! /* -1 in autovac setting means use plain vacuum_cost_delay */
! vac_cost_delay = (avopts && avopts->vacuum_cost_delay >= 0)
! ? avopts->vacuum_cost_delay
! : (autovacuum_vac_cost_delay >= 0)
! ? autovacuum_vac_cost_delay
! : VacuumCostDelay;
!
! /* 0 or -1 in autovac setting means use plain vacuum_cost_limit */
! vac_cost_limit = (avopts && avopts->vacuum_cost_limit > 0)
! ? avopts->vacuum_cost_limit
! : (autovacuum_vac_cost_limit > 0)
! ? autovacuum_vac_cost_limit
! : VacuumCostLimit;
!
! /* these do not have autovacuum-specific settings */
! freeze_min_age = (avopts && avopts->freeze_min_age >= 0)
! ? avopts->freeze_min_age
! : default_freeze_min_age;
!
! freeze_table_age = (avopts && avopts->freeze_table_age >= 0)
! ? avopts->freeze_table_age
! : default_freeze_table_age;
tab = palloc(sizeof(autovac_table));
tab->at_relid = relid;
*************** relation_needs_vacanalyze(Oid relid,
*** 2563,2587 ****
* sources: the passed reloptions (which could be a main table or a toast
* table), or the autovacuum GUC variables.
*/
! if (relopts)
! {
! vac_scale_factor = relopts->vacuum_scale_factor;
! vac_base_thresh = relopts->vacuum_threshold;
! anl_scale_factor = relopts->analyze_scale_factor;
! anl_base_thresh = relopts->analyze_threshold;
! freeze_max_age = Min(relopts->freeze_max_age,
! autovacuum_freeze_max_age);
! av_enabled = relopts->enabled;
! }
! else
! {
! vac_scale_factor = autovacuum_vac_scale;
! vac_base_thresh = autovacuum_vac_thresh;
! anl_scale_factor = autovacuum_anl_scale;
! anl_base_thresh = autovacuum_anl_thresh;
! freeze_max_age = autovacuum_freeze_max_age;
! av_enabled = true;
! }
/* Force vacuum if table is at risk of wraparound */
xidForceLimit = recentXid - freeze_max_age;
--- 2567,2595 ----
* sources: the passed reloptions (which could be a main table or a toast
* table), or the autovacuum GUC variables.
*/
!
! /* -1 in autovac setting means use plain vacuum_cost_delay */
! vac_scale_factor = (relopts && relopts->vacuum_scale_factor >= 0)
! ? relopts->vacuum_scale_factor
! : autovacuum_vac_scale;
!
! vac_base_thresh = (relopts && relopts->vacuum_threshold >= 0)
! ? relopts->vacuum_threshold
! : autovacuum_vac_thresh;
!
! anl_scale_factor = (relopts && relopts->analyze_scale_factor >= 0)
! ? relopts->analyze_scale_factor
! : autovacuum_anl_scale;
!
! anl_base_thresh = (relopts && relopts->analyze_threshold >= 0)
! ? relopts->analyze_threshold
! : autovacuum_anl_thresh;
!
! freeze_max_age = (relopts && relopts->freeze_max_age >= 0)
! ? Min(relopts->freeze_max_age, autovacuum_freeze_max_age)
! : autovacuum_freeze_max_age;
!
! av_enabled = (relopts ? relopts->enabled : true);
/* Force vacuum if table is at risk of wraparound */
xidForceLimit = recentXid - freeze_max_age;
Itagaki Takahiro wrote:
Alvaro Herrera <alvherre@commandprompt.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@commandprompt.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@commandprompt.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@commandprompt.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