pgsql: Update autovacuum to use reloptions instead of a system catalog,

Log Message:
-----------
Update autovacuum to use reloptions instead of a system catalog, for
per-table overrides of parameters.

This removes a whole class of problems related to misusing the catalog,
and perhaps more importantly, gives us pg_dump support for the parameters.

Based on a patch by Euler Taveira de Oliveira, heavily reworked by me.

Modified Files:
--------------
pgsql/doc/src/sgml:
catalogs.sgml (r2.196 -> r2.197)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/doc/src/sgml/catalogs.sgml?r1=2.196&r2=2.197)
config.sgml (r1.206 -> r1.207)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/doc/src/sgml/config.sgml?r1=1.206&r2=1.207)
maintenance.sgml (r1.89 -> r1.90)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/doc/src/sgml/maintenance.sgml?r1=1.89&r2=1.90)
pgsql/doc/src/sgml/ref:
alter_table.sgml (r1.102 -> r1.103)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/doc/src/sgml/ref/alter_table.sgml?r1=1.102&r2=1.103)
create_table.sgml (r1.112 -> r1.113)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/doc/src/sgml/ref/create_table.sgml?r1=1.112&r2=1.113)
pgsql/src/backend/access/common:
reloptions.c (r1.20 -> r1.21)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/access/common/reloptions.c?r1=1.20&r2=1.21)
pgsql/src/backend/catalog:
Makefile (r1.68 -> r1.69)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/catalog/Makefile?r1=1.68&r2=1.69)
pgsql/src/backend/postmaster:
autovacuum.c (r1.92 -> r1.93)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/postmaster/autovacuum.c?r1=1.92&r2=1.93)
pgsql/src/include/catalog:
catversion.h (r1.521 -> r1.522)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/include/catalog/catversion.h?r1=1.521&r2=1.522)
indexing.h (r1.106 -> r1.107)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/include/catalog/indexing.h?r1=1.106&r2=1.107)
pgsql/src/include/utils:
rel.h (r1.111 -> r1.112)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/include/utils/rel.h?r1=1.111&r2=1.112)
pgsql/src/test/regress/expected:
sanity_check.out (r1.38 -> r1.39)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/test/regress/expected/sanity_check.out?r1=1.38&r2=1.39)

Removed Files:
-------------
pgsql/src/include/catalog:
pg_autovacuum.h
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/include/catalog/pg_autovacuum.h)

Hi,

alvherre(at)postgresql(dot)org (Alvaro Herrera) wrote:

> Log Message:
> -----------
> Update autovacuum to use reloptions instead of a system catalog, for
> per-table overrides of parameters.
>
> This removes a whole class of problems related to misusing the catalog,
> and perhaps more importantly, gives us pg_dump support for the parameters.
>
> Based on a patch by Euler Taveira de Oliveira, heavily reworked by me.

I tested this changes and found two issues:

1. fillfactor.* options are silently ignored when the table doesn't have
toast relation. Should we notice the behabior to users?
ex. NOTICE: toast storage parameters are ignored
because the table doesn't have toast relations.

2. psql's \d+ doesn't show toast storage parameters.

Neither \d+ for base tables nor toast relations show toast.* parameters
though there are some values in pg_class.reloptions.
I think we should show toast.* parameters in \d+ for base tables
because it has consistency; we set them at ALTER TABLE for base tables.

=# CREATE TABLE tbl (t text) WITH (fillfactor=90, toast.fillfactor=70);
=# SELECT 'tbl'::regclass::oid;
oid
-------
16388

=# \d+ tbl
Table "public.tbl"
Column | Type | Modifiers | Storage | Description
--------+------+-----------+----------+-------------
t | text | | extended |
Has OIDs: no
Options: fillfactor=90

*** Should we show toast.fillfactor=70 here? ***

=# \d+ pg_toast.pg_toast_16388
TOAST table "pg_toast.pg_toast_16388"
Column | Type | Storage | Description
------------+---------+---------+-------------
chunk_id | oid | plain |
chunk_seq | integer | plain |
chunk_data | bytea | plain |

*** No descriptions about options here. ***

=# SELECT oid, relname, reloptions FROM pg_class
WHERE oid = 'pg_toast.pg_toast_16388'::regclass;
oid | relname | reloptions
-------+----------------+-----------------
16391 | pg_toast_16388 | {fillfactor=70}

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

ITAGAKI Takahiro escreveu:
> 1. fillfactor.* options are silently ignored when the table doesn't have
> toast relation. Should we notice the behabior to users?
> ex. NOTICE: toast storage parameters are ignored
> because the table doesn't have toast relations.
>
It was discussed and rejected [1].

> I think we should show toast.* parameters in \d+ for base tables
> because it has consistency; we set them at ALTER TABLE for base tables.
>
+1. That's because the psql patch was applied _before_ the namespace patch. It
seems we will need to hardcode the namespace notion at psql code.

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

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

ITAGAKI Takahiro wrote:

> I tested this changes and found two issues:
>
> 1. fillfactor.* options are silently ignored when the table doesn't have
> toast relation. Should we notice the behabior to users?
> ex. NOTICE: toast storage parameters are ignored
> because the table doesn't have toast relations.

You mean "toast.* options"? If so, yes, they are silently ignored.
Maybe issuing a warning is not a bad idea. Care to propose a patch?

> 2. psql's \d+ doesn't show toast storage parameters.
>
> Neither \d+ for base tables nor toast relations show toast.* parameters
> though there are some values in pg_class.reloptions.

Yeah, this is a bug in psql. I neglected to update \d+ when I committed
the namespace patch. I'll investigate.

ITAGAKI Takahiro wrote:

> 2. psql's \d+ doesn't show toast storage parameters.
>
> Neither \d+ for base tables nor toast relations show toast.* parameters
> though there are some values in pg_class.reloptions.
> I think we should show toast.* parameters in \d+ for base tables
> because it has consistency; we set them at ALTER TABLE for base tables.

This patch seems to fix this problem.

Note that it introduces a LEFT JOIN on pg_class to itself that's always
present, even for server versions that do not support reloptions. I'm
not sure that this is a problem; I can't measure any difference on \d
with and without the patch on a test database with 1000 tables.

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Note that it introduces a LEFT JOIN on pg_class to itself that's always
> present, even for server versions that do not support reloptions.

Personally I'd be more worried about the unnest(). Also, please
schema-qualify that function name; you can't assume anything about
the search path here.

regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> > Note that it introduces a LEFT JOIN on pg_class to itself that's always
> > present, even for server versions that do not support reloptions.
>
> Personally I'd be more worried about the unnest(). Also, please
> schema-qualify that function name; you can't assume anything about
> the search path here.

This version should fix these issues. I refrained from adding more ? :
expressions because it starts getting ugly for my taste.

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
>> Note that it introduces a LEFT JOIN on pg_class to itself that's always
>> present, even for server versions that do not support reloptions.
>
> Personally I'd be more worried about the unnest(). Also, please
> schema-qualify that function name; you can't assume anything about
> the search path here.

Maybe it would be more elegant to put the search_path into proconfig?
This would have some advantages:

0. Looks less weird.

1. We could quasi-automatically verify that all SQL-language functions
have the correct search path (or even set it in initdb).

2. On things like unnest, which is a language element that we happen to
implement as a function now, you don't have to worry about it one way or
the other.

In a shell script, you'd usually set the path at the top instead of
writing out the directories of every command. It looks better (reason
0), it's easier to analyze (e.g., lintian) (reason 1), and it avoids
confusion with shell built-ins (reason 2).

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> Tom Lane wrote:
>> Personally I'd be more worried about the unnest(). Also, please
>> schema-qualify that function name; you can't assume anything about
>> the search path here.

> Maybe it would be more elegant to put the search_path into proconfig?

This is psql's describe.c, not a sql function. It hasn't got a
proconfig, and it doesn't seem like a particularly good idea to make
it try to replace the user's search path setting directly.

regards, tom lane

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> This version should fix these issues. I refrained from adding more ? :
> expressions because it starts getting ugly for my taste.

I think you might as well just introduce two separate code paths to
produce the 8.4 and pre-8.4 queries, so that you don't need the LEFT
JOIN in the pre-8.4 path. IMHO the cut-and-paste way that we usually
do it in pg_dump is a whole lot easier to read and maintain than this
sort of ?: spaghetti.

regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> > This version should fix these issues. I refrained from adding more ? :
> > expressions because it starts getting ugly for my taste.
>
> I think you might as well just introduce two separate code paths to
> produce the 8.4 and pre-8.4 queries, so that you don't need the LEFT
> JOIN in the pre-8.4 path.

Right, see attached. (Separating the last two cases is probably
overkill ...?) I tested with HEAD, 8.2 and 8.0, seems to work fine.

> IMHO the cut-and-paste way that we usually
> do it in pg_dump is a whole lot easier to read and maintain than this
> sort of ?: spaghetti.

Hmm, so should we try to get rid of them in a more consistent fashion?

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Tom Lane wrote:
>> IMHO the cut-and-paste way that we usually
>> do it in pg_dump is a whole lot easier to read and maintain than this
>> sort of ?: spaghetti.

> Hmm, so should we try to get rid of them in a more consistent fashion?

If you've got the time and interest to work on the rest of describe.c,
it'd be fine with me. I don't feel a compulsion to go fix the rest
right now, though. It just seemed that this particular query had gotten
out of hand.

regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> > Tom Lane wrote:
> >> IMHO the cut-and-paste way that we usually
> >> do it in pg_dump is a whole lot easier to read and maintain than this
> >> sort of ?: spaghetti.
>
> > Hmm, so should we try to get rid of them in a more consistent fashion?
>
> If you've got the time and interest to work on the rest of describe.c,
> it'd be fine with me. I don't feel a compulsion to go fix the rest
> right now, though. It just seemed that this particular query had gotten
> out of hand.

Yeah, I think we can get away with fixing the queries one by one as we
go over them in future psql improvements. I have committed this for
now.

Alvaro Herrera wrote:
> ITAGAKI Takahiro wrote:
>
> > I tested this changes and found two issues:
> >
> > 1. fillfactor.* options are silently ignored when the table doesn't have
> > toast relation. Should we notice the behabior to users?
> > ex. NOTICE: toast storage parameters are ignored
> > because the table doesn't have toast relations.
>
> You mean "toast.* options"? If so, yes, they are silently ignored.
> Maybe issuing a warning is not a bad idea. Care to propose a patch?

Any takers here?

The second issue has been solved.

Thanks for testing.

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Alvaro Herrera wrote:
>> ITAGAKI Takahiro wrote:
>>> 1. fillfactor.* options are silently ignored when the table doesn't have
>>> toast relation. Should we notice the behabior to users?
>>> ex. NOTICE: toast storage parameters are ignored

>> You mean "toast.* options"? If so, yes, they are silently ignored.
>> Maybe issuing a warning is not a bad idea. Care to propose a patch?

> Any takers here?

I tend to think this isn't a very good idea. It's difficult for
applications to know whether a toast table will be created or not.
They should be able to just set the toast options and not worry.

regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> > Alvaro Herrera wrote:
> >> ITAGAKI Takahiro wrote:
> >>> 1. fillfactor.* options are silently ignored when the table doesn't have
> >>> toast relation. Should we notice the behabior to users?
> >>> ex. NOTICE: toast storage parameters are ignored
>
> >> You mean "toast.* options"? If so, yes, they are silently ignored.
> >> Maybe issuing a warning is not a bad idea. Care to propose a patch?
>
> > Any takers here?
>
> I tend to think this isn't a very good idea. It's difficult for
> applications to know whether a toast table will be created or not.
> They should be able to just set the toast options and not worry.

The problem is where do we store the options?

Alvaro Herrera wrote:
> Tom Lane wrote:
>> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
>>> Alvaro Herrera wrote:
>>>> ITAGAKI Takahiro wrote:
>>>>> 1. fillfactor.* options are silently ignored when the table doesn't have
>>>>> toast relation. Should we notice the behabior to users?
>>>>> ex. NOTICE: toast storage parameters are ignored
>>>> You mean "toast.* options"? If so, yes, they are silently ignored.
>>>> Maybe issuing a warning is not a bad idea. Care to propose a patch?
>>> Any takers here?
>> I tend to think this isn't a very good idea. It's difficult for
>> applications to know whether a toast table will be created or not.
>> They should be able to just set the toast options and not worry.
>
> The problem is where do we store the options?

How about in the reloptions of the main relation?

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas wrote:
> Alvaro Herrera wrote:
>> Tom Lane wrote:

>>> I tend to think this isn't a very good idea. It's difficult for
>>> applications to know whether a toast table will be created or not.
>>> They should be able to just set the toast options and not worry.
>>
>> The problem is where do we store the options?
>
> How about in the reloptions of the main relation?

Yes, perhaps that could be made to work.

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Tom Lane wrote:
>> I tend to think this isn't a very good idea. It's difficult for
>> applications to know whether a toast table will be created or not.
>> They should be able to just set the toast options and not worry.

> The problem is where do we store the options?

We don't. If there's no toast table, we don't need them after all.

Now the alternative position you could take is that if someone is
setting toast reloptions, they should darn well know the implementation
well enough to know whether the table will have a toast table or not.
In which case you should argue that this case ought to be an ERROR,
not a notice or warning. But I think that's probably unsustainably
anal. For example consider the following scenario:

create table foo (f1 int, f2 text);
set some toast reloptions on foo
alter table foo drop column f2;

pg_dump

At this point foo still has a toast table and presumably pg_dump will
dump its options. At reload, however, no toast table will be created,
and so throwing an error would be pretty embarrassing.

It's not hard to scale this up to find situations where the creation
of a toast table would be platform- or version-dependent (if the max
tuple width is just under a page).

If we are not able to teach pg_dump to predict whether the target
DB will create a toast table, we certainly can't expect applications
to know it. So I think setting toast reloptions on a table that has
no toast table should just be a silent no-op.

regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> > Tom Lane wrote:
> >> I tend to think this isn't a very good idea. It's difficult for
> >> applications to know whether a toast table will be created or not.
> >> They should be able to just set the toast options and not worry.
>
> > The problem is where do we store the options?
>
> We don't. If there's no toast table, we don't need them after all.

Well, that's the position that the current code is taking.

However, Takahiro-san and Euler's position is that if you do this:

create table foo (f1 int) with (toast.fillfactor = 70);
alter table foo add column f2 text;

Then the toast table should have the fillfactor setting. Right now they
are lost.

If we agree that the options are OK to be lost, then there's nothing we
need to do (and that's my opinion). If we don't, then we need some
weird hack to make it work somehow. Personally I think that it needs a
lot more work than it warrants.

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> However, Takahiro-san and Euler's position is that if you do this:
> create table foo (f1 int) with (toast.fillfactor = 70);
> alter table foo add column f2 text;
> Then the toast table should have the fillfactor setting.

Well, that might look sensible when phrased that way. But the more
likely scenario would be that you add column f2 six months later,
at which point there is room for pretty serious doubt that the option
you specified way back when would still be the optimal choice. I'm
just fine with the concept that if ADD COLUMN causes a toast table
to get created, that table will have default reloptions. If you want
nondefault toast reloptions, having to specify what you want after
the table exists (and you know what's in it) seems reasonable to me.

Or to put it another way: it seems to me that the use-case being argued
here is really for being able to adjust the default toast reloptions.
Not to have action at a distance on a table that doesn't exist and you
have no way to know when you set the option what will be in it when it
does exist.

regards, tom lane

Tom Lane wrote:

> Or to put it another way: it seems to me that the use-case being argued
> here is really for being able to adjust the default toast reloptions.
> Not to have action at a distance on a table that doesn't exist and you
> have no way to know when you set the option what will be in it when it
> does exist.

This argument makes perfect sense to me.

Since we don't have a way to set default reloptions for main tables
either, I don't think we should be pushing very hard for having one to
set default reloptions for toast tables.

Even if we were to argue that we should have both, it doesn't seem
material for 8.4.

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Since we don't have a way to set default reloptions for main tables
> either, I don't think we should be pushing very hard for having one to
> set default reloptions for toast tables.

> Even if we were to argue that we should have both, it doesn't seem
> material for 8.4.

Seems like a reasonable TODO entry, though. I think the use-case for
such a default mechanism is a bit thin today, but if we continue to
push more functionality into reloptions it's going to become valuable.

regards, tom lane

On Wed, Feb 11, 2009 at 3:40 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
>> However, Takahiro-san and Euler's position is that if you do this:
>> create table foo (f1 int) with (toast.fillfactor = 70);
>> alter table foo add column f2 text;
>> Then the toast table should have the fillfactor setting.
>
> Well, that might look sensible when phrased that way. But the more
> likely scenario would be that you add column f2 six months later,
> at which point there is room for pretty serious doubt that the option
> you specified way back when would still be the optimal choice. I'm
> just fine with the concept that if ADD COLUMN causes a toast table
> to get created, that table will have default reloptions. If you want
> nondefault toast reloptions, having to specify what you want after
> the table exists (and you know what's in it) seems reasonable to me.

FWIW, I don't really buy this argument. I can't see that it's all
that implausible to think that the user might be able to prognosticate
a reasonable value for a future TOAST table. The fact that they may
end up being wrong is hardly grounds to silently ignore whatever value
they tell us they want.

On the other hand, since I've never had a reason to tune this knob
myself, for TOAST tables or otherwise, I can't say I'm feeling a
violent urge to be the one to fix it.

...Robert

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> FWIW, I don't really buy this argument. I can't see that it's all
> that implausible to think that the user might be able to prognosticate
> a reasonable value for a future TOAST table.

Well, it still seems to me that such a user is really more interested in
a way to set the default toast fillfactor (or whatever option is under
discussion), ie what he really knows is a reasonable value for *all*
future TOAST tables in his installation.

Otherwise you're arguing that he knows exactly what the fillfactor
should be for a specific toast table and not any other one ... except
he doesn't know when that toast table is going to be created, which
calls into question the quality of his judgment about its specific
behavior otherwise.

regards, tom lane

On Wed, Feb 11, 2009 at 4:42 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> FWIW, I don't really buy this argument. I can't see that it's all
>> that implausible to think that the user might be able to prognosticate
>> a reasonable value for a future TOAST table.
>
> Well, it still seems to me that such a user is really more interested in
> a way to set the default toast fillfactor (or whatever option is under
> discussion), ie what he really knows is a reasonable value for *all*
> future TOAST tables in his installation.

Maybe, or maybe he knows that this group of tables is typically pretty
stable, but this group over here has more frequent updates, so
different fillfactors are appropriate... doesn't have to be 100%
site-wide.

> Otherwise you're arguing that he knows exactly what the fillfactor
> should be for a specific toast table and not any other one ... except
> he doesn't know when that toast table is going to be created, which
> calls into question the quality of his judgment about its specific
> behavior otherwise.

Sure, but I think you're putting too much emphasis on the likely
quality of the user's judgment. It's not really the place of the
database to ignore user requests, even if they're likely stupid
requests.

WARNING: Type varchar(9) is likely inadequate for assumed purpose of
column `telephone_number'.

...Robert