Re: reloptions with a "namespace"

Attached is a patch that adds a namespace capability to reloptions. It
works this way:

alter table foo set (namespace.option = value)

Currently the only namespace that does anything is "toast". What it
does is store the option in the toast table pg_class.reloptions.

It works fine, i.e. I can set a toast table fillfactor with a command
that references the main table. I am also able to do this:
CREATE TABLE foo (a int, b text) WITH (fillfactor = 80, toast.fillfactor = 75);
and it correctly sets the fillfactor for both tables.

This uses a new parse node. One possible disadvantage is that a command
like this works, but does nothing:

alvherre=# alter table foo set (test.foo = 1);
ALTER TABLE

This now needs pg_dump support to be complete.

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

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> This uses a new parse node.

You need at least equalfuncs.c support for that, and outfuncs would be
advisable.

> One possible disadvantage is that a command
> like this works, but does nothing:
> alvherre=# alter table foo set (test.foo = 1);

Uh, why doesn't it throw an error? We throw error for unrecognized
reloptions in general.

regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> > This uses a new parse node.
>
> You need at least equalfuncs.c support for that, and outfuncs would be
> advisable.

Added.

> > One possible disadvantage is that a command
> > like this works, but does nothing:
> > alvherre=# alter table foo set (test.foo = 1);
>
> Uh, why doesn't it throw an error? We throw error for unrecognized
> reloptions in general.

I wasn't sure of the best place to add a check. I have added it to
transformRelOptions; I am not entirely comfortable with it, because it
works, but it still allows this:

alvherre=# alter index f set (toast.fillfactor = 20);
ALTER INDEX

The original case now fails correctly with

alvherre=# alter table foo set (test.foo = 1);
ERROR: 22023: unrecognized parameter namespace "test"
UBICACIÓN: transformRelOptions, /pgsql/source/04toastopt/src/backend/access/common/reloptions.c:490
alvherre=# alter table foo set (toast.foo = 1);
ERROR: 22023: unrecognized parameter "foo"
UBICACIÓN: parseRelOptions, /pgsql/source/04toastopt/src/backend/access/common/reloptions.c:694

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

Alvaro Herrera escreveu:
> I wasn't sure of the best place to add a check. I have added it to
> transformRelOptions; I am not entirely comfortable with it, because it
> works, but it still allows this:
>
IMHO it's the appropriate place.

> alvherre=# alter index f set (toast.fillfactor = 20);
> ALTER INDEX
>
Maybe you need to add relopt_kind test in your validation code.

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

Euler Taveira de Oliveira wrote:
> Alvaro Herrera escreveu:
> > I wasn't sure of the best place to add a check. I have added it to
> > transformRelOptions; I am not entirely comfortable with it, because it
> > works, but it still allows this:
> >
> IMHO it's the appropriate place.

I think the best place would be parseRelOptions. The problem is that
transformRelOptions does not apply any semantics to the values it's
parsing; it doesn't know about the relopt_kind for example. That stuff
is only known by parseRelOptions, but when the options reach that point,
they have already lost the namespace (due to transformRelOptions).

> > alvherre=# alter index f set (toast.fillfactor = 20);
> > ALTER INDEX
> >
> Maybe you need to add relopt_kind test in your validation code.

No clean way to do that :-(

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

Alvaro Herrera escreveu:
> Euler Taveira de Oliveira wrote:
>> Alvaro Herrera escreveu:
>>> I wasn't sure of the best place to add a check. I have added it to
>>> transformRelOptions; I am not entirely comfortable with it, because it
>>> works, but it still allows this:
>>>
>> IMHO it's the appropriate place.
>
> I think the best place would be parseRelOptions. The problem is that
> transformRelOptions does not apply any semantics to the values it's
> parsing; it doesn't know about the relopt_kind for example. That stuff
> is only known by parseRelOptions, but when the options reach that point,
> they have already lost the namespace (due to transformRelOptions).
>
[Looking at your patch...] You're right. The only command that doesn't use
parseRelOptions() is ALTER INDEX. Is it the case to add a call at
index_reloptions() too? If it is not the case, I think you need to pass
'nmspc.relopt=value' instead of 'relopt=value' at transformRelOptions(). Of
course, you'll need to add some code at index_reloptions() to validate reloptions.

The following message doesn't say much. Isn't it the case to replace
'opt_definition' with 'OptWith' variant at gram.y?

euler=# create index fooi on foo(a) with (nmspc.relopt = 32);
ERROR: syntax error at or near "."
LINHA 1: create index fooi on foo(a) with (nmspc.relopt = 32);
^

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

Alvaro Herrera wrote:
> Euler Taveira de Oliveira wrote:
> > Alvaro Herrera escreveu:
> > > I wasn't sure of the best place to add a check. I have added it to
> > > transformRelOptions; I am not entirely comfortable with it, because it
> > > works, but it still allows this:
> > >
> > IMHO it's the appropriate place.
>
> I think the best place would be parseRelOptions. The problem is that
> transformRelOptions does not apply any semantics to the values it's
> parsing; it doesn't know about the relopt_kind for example. That stuff
> is only known by parseRelOptions, but when the options reach that point,
> they have already lost the namespace (due to transformRelOptions).

Okay, so I've changed things so that the transformRelOptions' caller is
now in charge of passing an array of valid option namespaces. This is
working A-OK. I'm now going to figure out appropriate pg_dump support
and commit as soon as possible.

--
Alvaro Herrera http://www.advogato.org/person/alvherre
"No es bueno caminar con un hombre muerto"

Alvaro Herrera escreveu:
> Alvaro Herrera wrote:
>> Euler Taveira de Oliveira wrote:
>>> Alvaro Herrera escreveu:
>>>> I wasn't sure of the best place to add a check. I have added it to
>>>> transformRelOptions; I am not entirely comfortable with it, because it
>>>> works, but it still allows this:
>>>>
>>> IMHO it's the appropriate place.
>> I think the best place would be parseRelOptions. The problem is that
>> transformRelOptions does not apply any semantics to the values it's
>> parsing; it doesn't know about the relopt_kind for example. That stuff
>> is only known by parseRelOptions, but when the options reach that point,
>> they have already lost the namespace (due to transformRelOptions).
>
> Okay, so I've changed things so that the transformRelOptions' caller is
> now in charge of passing an array of valid option namespaces. This is
> working A-OK. I'm now going to figure out appropriate pg_dump support
> and commit as soon as possible.
>
I don't like the spreading validnsps' approach. Isn't there a way to
centralize those variables in one place, i.e., reloption.h ? Also, remove an
obsolete comment about toast tables at reloption.h.

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

Euler Taveira de Oliveira wrote:
> Alvaro Herrera escreveu:

> > Okay, so I've changed things so that the transformRelOptions' caller is
> > now in charge of passing an array of valid option namespaces. This is
> > working A-OK. I'm now going to figure out appropriate pg_dump support
> > and commit as soon as possible.
> >
> I don't like the spreading validnsps' approach. Isn't there a way to
> centralize those variables in one place, i.e., reloption.h ? Also, remove an
> obsolete comment about toast tables at reloption.h.

No, that doesn't work, because we don't know centrally what's the
allowed list of namespaces. In fact that's precisely the problem: for
example, there's no point in having a "toast" namespace for index
reloptions. And for a user-defined access method, we don't know what
the valid namespaces are. Of course, the easiest way is to just state
that there are no valid namespaces other than NULL, and only allow
"toast" for heap, but I think that's not thinking far enough ahead.

The other option I considered was to have another AM entry point that
returns the list of valid namespaces, but that seems to be way overkill,
particularly considering that the current arrays are all NULL.

--
Alvaro Herrera http://www.amazon.com/gp/registry/CTMLCN8V17R4
"La gente vulgar solo piensa en pasar el tiempo;
el que tiene talento, en aprovecharlo"

New patch attached, with pg_dump support (thanks to Tom for the SQL
heads-up).

Euler Taveira de Oliveira wrote:

> I don't like the spreading validnsps' approach. Isn't there a way to
> centralize those variables in one place, i.e., reloption.h ?

Maybe one option is to create a #define with the options valid for
heaps?

> Also, remove an obsolete comment about toast tables at reloption.h.

I'm not sure about that one -- maybe one day we'll want to separate the
options for toast tables and those for plain tables (for example, surely
we don't need per-row default security in toast tables, or stuff like
that).

--
Alvaro Herrera http://www.amazon.com/gp/registry/CTMLCN8V17R4
"The West won the world not by the superiority of its ideas or values
or religion but rather by its superiority in applying organized violence.
Westerners often forget this fact, non-Westerners never do."
(Samuel P. Huntington)

Alvaro Herrera escreveu:
> New patch attached, with pg_dump support (thanks to Tom for the SQL
> heads-up).
>
Great! We're close. Just two minor gripes:

+ char *validnsps[] = { "toast" };

Surely, you forgot to add a NULL at the end. Patch is attached.

IIRC, my last patch includes a partial validation code for RESET cases. For
example, the last SQL will not be atomic (invalid reloption silently ignored).
So, why not apply the namespace validation code to RESET case too? Patch is
attached too. It does not handle the reloptions validation because the relOpts
initialization code is at parseRelOptions(); i leave it for a future refactor.

euler=# create table foo (a text) with (fillfactor=10);
CREATE TABLE
euler=# \d+ foo
Tabela "public.foo"
Coluna | Tipo | Modificadores | Storage | Descrição
--------+------+---------------+----------+-----------
a | text | | extended |
Têm OIDs: não
Options: fillfactor=10

euler=# alter table foo reset (fillfactor,foo.fillfactor);
ALTER TABLE
euler=# \d+ foo
Tabela "public.foo"
Coluna | Tipo | Modificadores | Storage | Descrição
--------+------+---------------+----------+-----------
a | text | | extended |
Têm OIDs: não

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

Euler Taveira de Oliveira escreveu:

[Forgot the first patch...]

> Alvaro Herrera escreveu:
>> New patch attached, with pg_dump support (thanks to Tom for the SQL
>> heads-up).
>>
> Great! We're close. Just two minor gripes:
>
> + char *validnsps[] = { "toast" };
>
> Surely, you forgot to add a NULL at the end. Patch is attached.
>
> IIRC, my last patch includes a partial validation code for RESET cases. For
> example, the last SQL will not be atomic (invalid reloption silently ignored).
> So, why not apply the namespace validation code to RESET case too? Patch is
> attached too. It does not handle the reloptions validation because the relOpts
> initialization code is at parseRelOptions(); i leave it for a future refactor.
>
> euler=# create table foo (a text) with (fillfactor=10);
> CREATE TABLE
> euler=# \d+ foo
> Tabela "public.foo"
> Coluna | Tipo | Modificadores | Storage | Descrição
> --------+------+---------------+----------+-----------
> a | text | | extended |
> Têm OIDs: não
> Options: fillfactor=10
>
> euler=# alter table foo reset (fillfactor,foo.fillfactor);
> ALTER TABLE
> euler=# \d+ foo
> Tabela "public.foo"
> Coluna | Tipo | Modificadores | Storage | Descrição
> --------+------+---------------+----------+-----------
> a | text | | extended |
> Têm OIDs: não
>
>
>
>
> ------------------------------------------------------------------------
>
>

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

Euler Taveira de Oliveira wrote:
> Alvaro Herrera escreveu:
> > New patch attached, with pg_dump support (thanks to Tom for the SQL
> > heads-up).
> >
> Great! We're close. Just two minor gripes:
>
> + char *validnsps[] = { "toast" };
>
> Surely, you forgot to add a NULL at the end. Patch is attached.

Right, thanks.

> IIRC, my last patch includes a partial validation code for RESET cases. For
> example, the last SQL will not be atomic (invalid reloption silently ignored).
> So, why not apply the namespace validation code to RESET case too? Patch is
> attached too.

No, we must not validate the options passed to RESET, because we want to
be able to reset even options that we do not currently think that are
valid. Consider that we might be trying to clean up after options set
by a previous version of a module.

--
Alvaro Herrera http://www.PlanetPostgreSQL.org/
"Saca el libro que tu religión considere como el indicado para encontrar la
oración que traiga paz a tu alma. Luego rebootea el computador
y ve si funciona" (Carlos Duclós)

Alvaro Herrera escreveu:
>> IIRC, my last patch includes a partial validation code for RESET cases. For
>> example, the last SQL will not be atomic (invalid reloption silently ignored).
>> So, why not apply the namespace validation code to RESET case too? Patch is
>> attached too.
>
> No, we must not validate the options passed to RESET, because we want to
> be able to reset even options that we do not currently think that are
> valid. Consider that we might be trying to clean up after options set
> by a previous version of a module.
>
Ah, idea withdrawn. But we should at least document this behavior.

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

Euler Taveira de Oliveira wrote:
> Alvaro Herrera escreveu:
> >> IIRC, my last patch includes a partial validation code for RESET cases. For
> >> example, the last SQL will not be atomic (invalid reloption silently ignored).
> >> So, why not apply the namespace validation code to RESET case too? Patch is
> >> attached too.
> >
> > No, we must not validate the options passed to RESET, because we want to
> > be able to reset even options that we do not currently think that are
> > valid. Consider that we might be trying to clean up after options set
> > by a previous version of a module.
> >
> Ah, idea withdrawn. But we should at least document this behavior.

Well, it is documented -- see amoptions here
http://www.postgresql.org/docs/8.3/static/index-functions.html

The problem with this is that documentation for reloptions is scattered
all over the place. I think we should have a separate section somewhere
on which they are discussed at length.

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