fillfactor gets set to zero for toast tables
I've been able to reproduce the problem described here:
http://archives.postgresql.org/pgsql-bugs/2010-05/msg00100.php
Do this:
create table foo(f1 text);
alter table foo set (toast.autovacuum_enabled = false);
insert into foo values(repeat('xyzzy',100000));
vacuum verbose foo;
Notice that the vacuum output shows there's only one toast tuple per
toast table page.
The problem is that if any reloption is set for the toast table,
we build a StdRdOptions struct in which fillfactor is zero, and
then all the code that actually uses fillfactor honors that.
And the reason fillfactor gets left as zero is that there is no
entry for it in the tables for toast-table reloptions. Clearly,
this wasn't thought through carefully enough.
I'm inclined to think that we should remove the notion of there
being separate sets of rdoptions for heaps and toast tables ---
any case in which someone tries to omit a StdRdOption for toast
tables is going to fail the same way, unless we are fortunate
enough that zero is a desirable default. What seems more rational
is to provide toast.fillfactor but give it min/max/default = 100.
Thoughts?
regards, tom lane
Excerpts from Tom Lane's message of vie may 14 14:19:30 -0400 2010:
The problem is that if any reloption is set for the toast table,
we build a StdRdOptions struct in which fillfactor is zero, and
then all the code that actually uses fillfactor honors that.
And the reason fillfactor gets left as zero is that there is no
entry for it in the tables for toast-table reloptions. Clearly,
this wasn't thought through carefully enough.
Ouch :-( We messed with this stuff after the initial commit because I
didn't get it right the first time either. I'm surprised that we didn't
find this problem right away.
I'm inclined to think that we should remove the notion of there
being separate sets of rdoptions for heaps and toast tables ---
any case in which someone tries to omit a StdRdOption for toast
tables is going to fail the same way, unless we are fortunate
enough that zero is a desirable default.
It doesn't seem like having the distinction has bought us anything.
However, if we do that, we can't add a separate entry to intRelOpts to
fix min/max/default to 100, so I think we should document that options
for toast must match those for heaps.
What seems more rational is to provide toast.fillfactor but give it
min/max/default = 100.
Adding an entry to intRelOpts should be enough to fix this bug, correct?
--
Alvaro Herrera <alvherre@commandprompt.com> writes:
Excerpts from Tom Lane's message of vie may 14 14:19:30 -0400 2010:
What seems more rational is to provide toast.fillfactor but give it
min/max/default = 100.
Adding an entry to intRelOpts should be enough to fix this bug, correct?
I think so, but haven't tested. The docs would need some correction
perhaps.
BTW, I notice that the code allows people to fool with
autovacuum_analyze_threshold and related parameters for toast tables,
which seems rather pointless since we never analyze toast tables.
So the fillfactor isn't really the only instance of the issue.
Maybe a better solution is to have some kind of notion of a default-only
entry, which is sufficient to insert the default into the struct but
isn't accepted as a user-settable item.
regards, tom lane
Excerpts from Tom Lane's message of vie may 14 15:03:57 -0400 2010:
Maybe a better solution is to have some kind of notion of a default-only
entry, which is sufficient to insert the default into the struct but
isn't accepted as a user-settable item.
This patch (for 8.4, but applies fuzzily to 9.0) implements this idea.
Note that there's no explicit check that every heap option has a
corresponding toast option; that's left to the developer's judgement to
add. I added the new member to relopt_gen struct so that existing
entries did not require changes in initializers.
(I'll leave the error as ERRCODE_CANT_CHANGE_RUNTIME_PARAM unless
someone thinks ERRCODE_INVALID_PARAMETER_VALUE is better)
I checked and this is the only heap setting that did not have a toast
setting.
I'll leave this open to comments for a bit more time than usual, to give
room for pgcon beers and such ...
--
Attachments:
relopt-toast-ff.patchapplication/octet-stream; name=relopt-toast-ff.patchDownload
Index: src/backend/access/common/reloptions.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/access/common/reloptions.c,v
retrieving revision 1.28.2.2
diff -c -p -r1.28.2.2 reloptions.c
*** src/backend/access/common/reloptions.c 11 Mar 2010 21:47:25 -0000 1.28.2.2
--- src/backend/access/common/reloptions.c 18 May 2010 19:54:57 -0000
***************
*** 44,49 ****
--- 44,52 ----
*
* Note that we don't handle "oids" in relOpts because it is handled by
* interpretOidsOption().
+ *
+ * Note that any option that exists for RELOPT_KIND_HEAP must also exist for
+ * RELOPT_KIND_TOAST.
*/
static relopt_bool boolRelOpts[] =
*************** static relopt_int intRelOpts[] =
*** 81,86 ****
--- 84,97 ----
{
{
"fillfactor",
+ "Toast default fillfactor, cannot be changed",
+ RELOPT_KIND_TOAST, true
+ },
+ 100, 100, 100
+ },
+ {
+ {
+ "fillfactor",
"Packs btree index pages only to this percentage",
RELOPT_KIND_BTREE
},
*************** parse_one_reloption(relopt_value *option
*** 886,891 ****
--- 897,908 ----
errmsg("parameter \"%s\" specified more than once",
option->gen->name)));
+ if (option->gen->default_only && validate)
+ ereport(ERROR,
+ (errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM), /* ERRCODE_INVALID_PARAMETER_VALUE */
+ errmsg("parameter \"%s\" cannot be changed",
+ option->gen->name)));
+
value_len = text_len - option->gen->namelen - 1;
value = (char *) palloc(value_len + 1);
memcpy(value, text_str + option->gen->namelen + 1, value_len);
Index: src/include/access/reloptions.h
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/include/access/reloptions.h,v
retrieving revision 1.16
diff -c -p -r1.16 reloptions.h
*** src/include/access/reloptions.h 11 Jun 2009 14:49:09 -0000 1.16
--- src/include/access/reloptions.h 18 May 2010 20:43:40 -0000
*************** typedef struct relopt_gen
*** 55,60 ****
--- 55,61 ----
* marker) */
const char *desc;
bits32 kinds;
+ bool default_only; /* not settable? */
int namelen;
relopt_type type;
} relopt_gen;
Alvaro Herrera <alvherre@commandprompt.com> wrote:
Excerpts from Tom Lane's message of vie may 14 15:03:57 -0400 2010:
Maybe a better solution is to have some kind of notion of a default-only
entry, which is sufficient to insert the default into the struct but
isn't accepted as a user-settable item.This patch (for 8.4, but applies fuzzily to 9.0) implements this idea.
Note that there's no explicit check that every heap option has a
corresponding toast option; that's left to the developer's judgement to
add. I added the new member to relopt_gen struct so that existing
entries did not require changes in initializers.
The new "default_only" field can be initialized only from the internal codes
and is not exported to user definded reloptions. We could add an additional
argument to add_xxx_reloption() functions, but it breaks ABI.
How about the attached patch? It just fills fillfactor (and analyze_threshold)
to default values for TOAST relations. I think responsibility for filling
reloptions with proper values is not in the generic option routines but in
AM-specific reloption handlers.
Regards,
---
Takahiro Itagaki
NTT Open Source Software Center
Attachments:
toast-ff-fix.patchapplication/octet-stream; name=toast-ff-fix.patchDownload
Index: src/backend/access/common/reloptions.c
===================================================================
--- src/backend/access/common/reloptions.c (HEAD)
+++ src/backend/access/common/reloptions.c (fixed)
@@ -116,7 +116,7 @@
{
"autovacuum_analyze_threshold",
"Minimum number of tuple inserts, updates or deletes prior to analyze",
- RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+ RELOPT_KIND_HEAP
},
-1, 0, INT_MAX
},
@@ -1156,10 +1156,18 @@
bytea *
heap_reloptions(char relkind, Datum reloptions, bool validate)
{
+ StdRdOptions *rdopts;
+
switch (relkind)
{
case RELKIND_TOASTVALUE:
- return default_reloptions(reloptions, validate, RELOPT_KIND_TOAST);
+ rdopts = default_reloptions(reloptions, validate, RELOPT_KIND_TOAST);
+ if (rdopts != NULL)
+ {
+ rdopts->fillfactor = 100;
+ rdopts->autovacuum.analyze_threshold = -1;
+ }
+ return (bytea *) rdopts;
case RELKIND_RELATION:
return default_reloptions(reloptions, validate, RELOPT_KIND_HEAP);
default:
This is still an open item for 9.0, and we should also backport it to 8.4.
Which do we take on? Or is there better idea?
Alvaro's idea:
Maybe a better solution is to have some kind of notion of a default-only
entry, which is sufficient to insert the default into the struct but
isn't accepted as a user-settable item.
Itagaki's idea:
It just fills fillfactor (and analyze_threshold)
to default values for TOAST relations.
Regards,
---
Takahiro Itagaki
NTT Open Source Software Center
Excerpts from Takahiro Itagaki's message of mié may 26 03:32:56 -0400 2010:
Alvaro Herrera <alvherre@commandprompt.com> wrote:
Excerpts from Tom Lane's message of vie may 14 15:03:57 -0400 2010:
Maybe a better solution is to have some kind of notion of a default-only
entry, which is sufficient to insert the default into the struct but
isn't accepted as a user-settable item.This patch (for 8.4, but applies fuzzily to 9.0) implements this idea.
Note that there's no explicit check that every heap option has a
corresponding toast option; that's left to the developer's judgement to
add. I added the new member to relopt_gen struct so that existing
entries did not require changes in initializers.The new "default_only" field can be initialized only from the internal codes
and is not exported to user definded reloptions. We could add an additional
argument to add_xxx_reloption() functions, but it breaks ABI.
Do we really need default_only entries in user-defined reloptions?
We have yet to see any indication that anybody is using user-defined
reloptions at all ... It'd be good to have an use case at least (if
only to ensure that the API we're providing is sufficient).
If in the future we determine that we need to offer user-defined
default_only reloptions, perhaps we can add new entry points in the
reloptions API.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> writes:
Excerpts from Takahiro Itagaki's message of mié may 26 03:32:56 -0400 2010:
The new "default_only" field can be initialized only from the internal codes
and is not exported to user definded reloptions. We could add an additional
argument to add_xxx_reloption() functions, but it breaks ABI.
Do we really need default_only entries in user-defined reloptions?
We have yet to see any indication that anybody is using user-defined
reloptions at all ... It'd be good to have an use case at least (if
only to ensure that the API we're providing is sufficient).
There probably isn't anyone using them, yet, which seems to me to be
a good argument to fix any obvious deficiencies in the API *now*
before there actually is anyone who'll be affected. In particular,
I suggest that 9.0 would be a good time to add an "int flags" parameter
to the add_xxx_reloption functions. The first flag could be
default_only and we'd have room to add more later without another API
break.
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> wrote:
Alvaro Herrera <alvherre@commandprompt.com> writes:
Do we really need default_only entries in user-defined reloptions?
I think we don't, but I also think we don't need it at all even in the
core because it just set a few variables to the default values with
complex code flow. Could you explain why default_only entries idea is
better than adjusting those fields in the toast-specific codes?
It's my understanding that reloption-framework is just a tool to fill
reloption parameters, and it's not responsible for unused fields.
We have yet to see any indication that anybody is using user-defined
reloptions at all ... It'd be good to have an use case at least (if
only to ensure that the API we're providing is sufficient).
I use it my textsearch_senna extension :-).
But I don't need default_only entries at this time.
I suggest that 9.0 would be a good time to add an "int flags" parameter
to the add_xxx_reloption functions. The first flag could be
default_only and we'd have room to add more later without another API
break.
I agree the idea when we reach a conclusion to introduce default_only.
Regards,
---
Takahiro Itagaki
NTT Open Source Software Center
Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.jp> wrote:
Could you explain why default_only entries idea is
better than adjusting those fields in the toast-specific codes?
It's my understanding that reloption-framework is just a tool to fill
reloption parameters, and it's not responsible for unused fields.
Here is my proposal to fix the issue. I didn't introduce default_only
field but simply adjust parameters for TOAST reloptions.
BTW, not only heap and toast relations but also btree, hash, and gist
indexes use StdRdOptions. However, they actually don't support autovacuum
options. So, StdRdOptions is a bloated all-in-one descriptor now.
Instead, they should use fillfactor-only reloptions that is defined
separately from options for heap.
Regards,
---
Takahiro Itagaki
NTT Open Source Software Center
Attachments:
toast-default-only-relopts.patchapplication/octet-stream; name=toast-default-only-relopts.patchDownload
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index c5fa529..bcdeb36 100644
*** a/src/backend/access/common/reloptions.c
--- b/src/backend/access/common/reloptions.c
*************** static relopt_int intRelOpts[] =
*** 116,122 ****
{
"autovacuum_analyze_threshold",
"Minimum number of tuple inserts, updates or deletes prior to analyze",
! RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
},
-1, 0, INT_MAX
},
--- 116,122 ----
{
"autovacuum_analyze_threshold",
"Minimum number of tuple inserts, updates or deletes prior to analyze",
! RELOPT_KIND_HEAP
},
-1, 0, INT_MAX
},
*************** default_reloptions(Datum reloptions, boo
*** 1156,1165 ****
bytea *
heap_reloptions(char relkind, Datum reloptions, bool validate)
{
switch (relkind)
{
case RELKIND_TOASTVALUE:
! return default_reloptions(reloptions, validate, RELOPT_KIND_TOAST);
case RELKIND_RELATION:
return default_reloptions(reloptions, validate, RELOPT_KIND_HEAP);
default:
--- 1156,1175 ----
bytea *
heap_reloptions(char relkind, Datum reloptions, bool validate)
{
+ StdRdOptions *rdopts;
+
switch (relkind)
{
case RELKIND_TOASTVALUE:
! rdopts = (StdRdOptions *)
! default_reloptions(reloptions, validate, RELOPT_KIND_TOAST);
! if (rdopts != NULL)
! {
! /* adjust default-only parameters */
! rdopts->fillfactor = 100;
! rdopts->autovacuum.analyze_threshold = -1;
! }
! return (bytea *) rdopts;
case RELKIND_RELATION:
return default_reloptions(reloptions, validate, RELOPT_KIND_HEAP);
default: