Re: 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(at)commandprompt(dot)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 ...
--

Alvaro Herrera <alvherre(at)commandprompt(dot)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

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(at)commandprompt(dot)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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Alvaro Herrera <alvherre(at)commandprompt(dot)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(dot)takahiro(at)oss(dot)ntt(dot)co(dot)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