Thread: 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.
Attachment
Alvaro Herrera <alvherre@commandprompt.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@commandprompt.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.
Attachment
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"
Attachment
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)
Attachment
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/
--
-- PostgreSQL database dump
--
SET statement_timeout = 0;
SET client_encoding = 'UTF8';
SET standard_conforming_strings = off;
SET check_function_bodies = false;
SET client_min_messages = warning;
SET escape_string_warning = off;
--
-- Name: plperl; Type: PROCEDURAL LANGUAGE; Schema: -; Owner: euler
--
CREATE PROCEDURAL LANGUAGE plperl;
ALTER PROCEDURAL LANGUAGE plperl OWNER TO euler;
--
-- Name: plpgsql; Type: PROCEDURAL LANGUAGE; Schema: -; Owner: euler
--
CREATE PROCEDURAL LANGUAGE plpgsql;
ALTER PROCEDURAL LANGUAGE plpgsql OWNER TO euler;
--
-- Name: plpythonu; Type: PROCEDURAL LANGUAGE; Schema: -; Owner: euler
--
CREATE PROCEDURAL LANGUAGE plpythonu;
ALTER PROCEDURAL LANGUAGE plpythonu OWNER TO euler;
--
-- Name: pltcl; Type: PROCEDURAL LANGUAGE; Schema: -; Owner: euler
--
CREATE PROCEDURAL LANGUAGE pltcl;
ALTER PROCEDURAL LANGUAGE pltcl OWNER TO euler;
SET search_path = public, pg_catalog;
SET default_tablespace = '';
SET default_with_oids = false;
--
-- Name: tst1; Type: TABLE; Schema: public; Owner: euler; Tablespace:
--
CREATE TABLE tst1 (
a text
)
WITH (fillfactor=10);
ALTER TABLE public.tst1 OWNER TO euler;
--
-- Name: tst2; Type: TABLE; Schema: public; Owner: euler; Tablespace:
--
CREATE TABLE tst2 (
a text
)
WITH (toast.fillfactor=20);
ALTER TABLE public.tst2 OWNER TO euler;
--
-- Name: tst3; Type: TABLE; Schema: public; Owner: euler; Tablespace:
--
CREATE TABLE tst3 (
a text
)
WITH (fillfactor=10, toast.fillfactor=20);
ALTER TABLE public.tst3 OWNER TO euler;
--
-- Data for Name: tst1; Type: TABLE DATA; Schema: public; Owner: euler
--
COPY tst1 (a) FROM stdin;
\.
--
-- Data for Name: tst2; Type: TABLE DATA; Schema: public; Owner: euler
--
COPY tst2 (a) FROM stdin;
\.
--
-- Data for Name: tst3; Type: TABLE DATA; Schema: public; Owner: euler
--
COPY tst3 (a) FROM stdin;
\.
--
-- Name: public; Type: ACL; Schema: -; Owner: euler
--
REVOKE ALL ON SCHEMA public FROM PUBLIC;
REVOKE ALL ON SCHEMA public FROM euler;
GRANT ALL ON SCHEMA public TO euler;
GRANT ALL ON SCHEMA public TO PUBLIC;
--
-- PostgreSQL database dump complete
--
*** pgsql.alvaro/src/backend/access/common/reloptions.c 2009-01-31 02:01:21.000000000 -0200
--- pgsql.euler/src/backend/access/common/reloptions.c 2009-01-31 02:16:29.000000000 -0200
***************
*** 487,492 ****
--- 487,519 ----
{
ReloptElem *def = lfirst(cell);
+ /*
+ * Error out if the namespace is not valid. A NULL namespace
+ * is always valid.
+ */
+ if (def->nmspc != NULL)
+ {
+ bool valid = false;
+ int i;
+
+ if (validnsps)
+ {
+ for (i = 0; validnsps[i]; i++)
+ {
+ if (pg_strcasecmp(def->nmspc, validnsps[i]) == 0)
+ {
+ valid = true;
+ break;
+ }
+ }
+ }
+
+ if (!valid)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("unrecognized parameter namespace \"%s\"",
+ def->nmspc)));
+ }
if (isReset)
{
***************
*** 501,534 ****
const char *value;
Size len;
- /*
- * Error out if the namespace is not valid. A NULL namespace
- * is always valid.
- */
- if (def->nmspc != NULL)
- {
- bool valid = false;
- int i;
-
- if (validnsps)
- {
- for (i = 0; validnsps[i]; i++)
- {
- if (pg_strcasecmp(def->nmspc, validnsps[i]) == 0)
- {
- valid = true;
- break;
- }
- }
- }
-
- if (!valid)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("unrecognized parameter namespace \"%s\"",
- def->nmspc)));
- }
-
if (ignoreOids && pg_strcasecmp(def->optname, "oids") == 0)
continue;
--- 528,533 ----
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/
diff -cr pgsql.alvaro/src/backend/commands/tablecmds.c pgsql.euler/src/backend/commands/tablecmds.c
*** pgsql.alvaro/src/backend/commands/tablecmds.c 2009-01-31 02:01:22.000000000 -0200
--- pgsql.euler/src/backend/commands/tablecmds.c 2009-01-31 01:47:32.000000000 -0200
***************
*** 351,357 ****
Datum reloptions;
ListCell *listptr;
AttrNumber attnum;
! char *validnsps[] = { "toast" };
/*
* Truncate relname to appropriate length (probably a waste of time, as
--- 351,357 ----
Datum reloptions;
ListCell *listptr;
AttrNumber attnum;
! static char *validnsps[] = { "toast", NULL };
/*
* Truncate relname to appropriate length (probably a waste of time, as
***************
*** 6459,6465 ****
Datum repl_val[Natts_pg_class];
bool repl_null[Natts_pg_class];
bool repl_repl[Natts_pg_class];
! char *validnsps[] = { "toast" };
if (defList == NIL)
return; /* nothing to do */
--- 6459,6465 ----
Datum repl_val[Natts_pg_class];
bool repl_null[Natts_pg_class];
bool repl_repl[Natts_pg_class];
! static char *validnsps[] = { "toast", NULL };
if (defList == NIL)
return; /* nothing to do */
diff -cr pgsql.alvaro/src/backend/executor/execMain.c pgsql.euler/src/backend/executor/execMain.c
*** pgsql.alvaro/src/backend/executor/execMain.c 2009-01-31 02:01:22.000000000 -0200
--- pgsql.euler/src/backend/executor/execMain.c 2009-01-31 01:48:19.000000000 -0200
***************
*** 2832,2838 ****
Oid intoRelationId;
TupleDesc tupdesc;
DR_intorel *myState;
! char *validnsps[] = { "toast" };
Assert(into);
--- 2832,2838 ----
Oid intoRelationId;
TupleDesc tupdesc;
DR_intorel *myState;
! static char *validnsps[] = { "toast", NULL };
Assert(into);
Somente em pgsql.euler/src/backend/parser: gram.c
Somente em pgsql.euler/src/backend/parser: gram.h
Somente em pgsql.euler/src/backend/parser: scan.c
diff -cr pgsql.alvaro/src/backend/tcop/utility.c pgsql.euler/src/backend/tcop/utility.c
*** pgsql.alvaro/src/backend/tcop/utility.c 2009-01-31 02:01:22.000000000 -0200
--- pgsql.euler/src/backend/tcop/utility.c 2009-01-31 01:47:51.000000000 -0200
***************
*** 424,430 ****
if (IsA(stmt, CreateStmt))
{
Datum toast_options;
! char *validnsps[] = { "toast" };
/* Create the table itself */
relOid = DefineRelation((CreateStmt *) stmt,
--- 424,430 ----
if (IsA(stmt, CreateStmt))
{
Datum toast_options;
! static char *validnsps[] = { "toast", NULL };
/* Create the table itself */
relOid = DefineRelation((CreateStmt *) stmt,
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.
Hi,
I've noticed a difference in 8.3.7 vs 8.4 (via git branch -r)
behaviour
8.3
----
testdb=> create table foo (bar bigserial primary key with
(fillfactor=75));
NOTICE: CREATE TABLE will create implicit sequence "foo_bar_seq" for
serial column "foo.bar"
NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index
"foo_pkey" for table "foo"
CREATE TABLE
testdb=> \d foo; Table "public.foo"Column | Type | Modifiers
--------+--------+---------------------------------------------------bar | bigint | not null default
nextval('foo_bar_seq'::regclass)
Indexes: "foo_pkey" PRIMARY KEY, btree (bar) WITH (fillfactor=75)
testdb=>
8.4
----
testdb=> create table foo (bar bigserial primary key with
(fillfactor=75));
NOTICE: CREATE TABLE will create implicit sequence "foo_bar_seq" for
serial column "foo.bar"
ERROR: unrecognized parameter namespace "fillfactor"
testdb=>
Additionally, "ALTER TABLE ONLY foo ADD CONSTRAINT bar PRIMARY KEY
(baz) WITH (fillfactor=n);" doesn't work on 8.4, which is used by
pg_dumpall on tables created with a fill-factor in 8.3.
After some debugging in reloptions.c, I've realised that the issue is
caused by fillfactor not having a validnsps in transformRelOptions.
Adding an additional condition "&& (validnsps))" at line 595 or 612
appears to fix the issue.
Regards,
Khee Chin.
Khee Chin escribió: > After some debugging in reloptions.c, I've realised that the issue is > caused by fillfactor not having a validnsps in transformRelOptions. > Adding an additional condition "&& (validnsps))" at line 595 or 612 > appears to fix the issue. No, the bug is that they are passed as DefElem instead of RelOptElem. The right fix is in the grammar ... checking now. Thanks for the report. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera escribió: > Khee Chin escribió: > > > After some debugging in reloptions.c, I've realised that the issue is > > caused by fillfactor not having a validnsps in transformRelOptions. > > Adding an additional condition "&& (validnsps))" at line 595 or 612 > > appears to fix the issue. > > No, the bug is that they are passed as DefElem instead of RelOptElem. > The right fix is in the grammar ... checking now. This patch seems to be the right cure. (Some other users of opt_definition remain; I think it's just CREATE FUNCTION by now). -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Attachment
Alvaro Herrera <alvherre@commandprompt.com> writes:
> This patch seems to be the right cure. (Some other users of
> opt_definition remain; I think it's just CREATE FUNCTION by now).
Surely this will break other things. I find myself wondering why you
invented ReloptElem at all, instead of adding a field to DefElem.
regards, tom lane
Tom Lane escribió: > Alvaro Herrera <alvherre@commandprompt.com> writes: > > This patch seems to be the right cure. (Some other users of > > opt_definition remain; I think it's just CREATE FUNCTION by now). > > Surely this will break other things. I find myself wondering why you > invented ReloptElem at all, instead of adding a field to DefElem. I had to, precisely because it messes up other uses of DefElem ... For example, the grammar would allow CREATE FUNCTION ... WITH something.name = value which we certainly don't want. I don't expect to break anything else actually. Keep in mind that those "opt_definition" were there precisely to carry reloptions for the affected indexes. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Tom Lane escribi�:
>> Surely this will break other things. I find myself wondering why you
>> invented ReloptElem at all, instead of adding a field to DefElem.
> I had to, precisely because it messes up other uses of DefElem ...
> For example, the grammar would allow
> CREATE FUNCTION ... WITH something.name = value
> which we certainly don't want.
Well, you could still have separate productions that did or didn't allow
qualified names there (or perhaps better, have the code in
functioncmds.c reject qualified names). I think the use of two different
node types is going to result in duplicate coding and/or bugs deeper in
the system, however.
regards, tom lane
Tom Lane escribió: > Alvaro Herrera <alvherre@commandprompt.com> writes: > > Tom Lane escribi�: > >> Surely this will break other things. I find myself wondering why you > >> invented ReloptElem at all, instead of adding a field to DefElem. > > > I had to, precisely because it messes up other uses of DefElem ... > > > For example, the grammar would allow > > CREATE FUNCTION ... WITH something.name = value > > which we certainly don't want. > > Well, you could still have separate productions that did or didn't allow > qualified names there (or perhaps better, have the code in > functioncmds.c reject qualified names). I think the use of two different > node types is going to result in duplicate coding and/or bugs deeper in > the system, however. I think what drove me away from that (which I certainly considered at some point) was the existance of OptionDefElem. Maybe it would work to make RelOptElem similar to that, i.e. have a char *namespace and a DefElem? -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera escribió:
> Tom Lane escribió:
> > Well, you could still have separate productions that did or didn't allow
> > qualified names there (or perhaps better, have the code in
> > functioncmds.c reject qualified names). I think the use of two different
> > node types is going to result in duplicate coding and/or bugs deeper in
> > the system, however.
>
> I think what drove me away from that (which I certainly considered at
> some point) was the existance of OptionDefElem. Maybe it would work to
> make RelOptElem similar to that, i.e. have a char *namespace and a
> DefElem?
... but I don't really see that this buys much of anything. I think a
better answer to this kind of problem would be
*** src/backend/access/common/reloptions.c 24 Mar 2009 20:17:09 -0000 1.24
--- src/backend/access/common/reloptions.c 3 Apr 2009 19:43:35 -0000
*************** transformRelOptions(Datum oldOptions, Li
*** 574,579 ****
--- 574,580 ---- { ReloptElem *def = lfirst(cell);
+ Assert(IsA(def, ReloptElem)); if (isReset) {
--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera <alvherre@commandprompt.com> writes:
>> Well, you could still have separate productions that did or didn't allow
>> qualified names there (or perhaps better, have the code in
>> functioncmds.c reject qualified names). I think the use of two different
>> node types is going to result in duplicate coding and/or bugs deeper in
>> the system, however.
> I think what drove me away from that (which I certainly considered at
> some point) was the existance of OptionDefElem. Maybe it would work to
> make RelOptElem similar to that, i.e. have a char *namespace and a
> DefElem?
OptionDefElem? [ click click grep grep ]
Hmm, I can see that there was more than one round of dubious decisions
made while I was looking the other way :-(. I'm thinking maybe all
three of these should be folded together. Let me think about it a
bit more --- since I'm the one complaining, I guess it should be on
my head to fix it.
regards, tom lane
Alvaro Herrera <alvherre@commandprompt.com> writes:
> ... but I don't really see that this buys much of anything. I think a
> better answer to this kind of problem would be
> + Assert(IsA(def, ReloptElem));
Well, that will help to make wrong-node-type mistakes more obvious (at
least if you're running an assert-enabled build). It does nothing to
*fix* the mistakes. And it does nothing to eliminate duplicate coding
for essentially redundant node types, which is bothering me as well.
I don't like any of the changes I see in define.c since 8.3 ...
regards, tom lane