Re: [HACKERS] reloptions with a namespace
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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] reloptions with a namespace
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 Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] reloptions with a namespace
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 Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.
Index: src/backend/parser/gram.y
===
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/parser/gram.y,v
retrieving revision 2.660
diff -c -p -r2.660 gram.y
*** src/backend/parser/gram.y 7 Mar 2009 00:13:57 - 2.660
--- src/backend/parser/gram.y 3 Apr 2009 19:35:38 -
*** ColConstraintElem:
*** 2188,2194
n-indexspace = NULL;
$$ = (Node *)n;
}
! | UNIQUE opt_definition OptConsTableSpace
{
Constraint *n = makeNode(Constraint);
n-contype = CONSTR_UNIQUE;
--- 2188,2194
n-indexspace = NULL;
$$ = (Node *)n;
}
! | UNIQUE opt_reloptions OptConsTableSpace
{
Constraint *n = makeNode(Constraint);
n-contype = CONSTR_UNIQUE;
*** ColConstraintElem:
*** 2200,2206
n-indexspace = $3;
$$ = (Node *)n;
}
! | PRIMARY KEY opt_definition OptConsTableSpace
{
Constraint *n = makeNode(Constraint);
n-contype = CONSTR_PRIMARY;
--- 2200,2206
n-indexspace = $3;
$$ = (Node *)n;
}
! | PRIMARY KEY opt_reloptions OptConsTableSpace
{
Constraint *n = makeNode(Constraint);
n-contype = CONSTR_PRIMARY;
*** ConstraintElem:
*** 2363,2369
n-indexspace = NULL;
$$ = (Node *)n;
}
! | UNIQUE '(' columnList ')' opt_definition OptConsTableSpace
{
Constraint *n = makeNode(Constraint);
n-contype = CONSTR_UNIQUE;
--- 2363,2369
n-indexspace = NULL;
$$ = (Node *)n;
}
! | UNIQUE '(' columnList ')' opt_reloptions OptConsTableSpace
{
Constraint *n = makeNode(Constraint);
n-contype = CONSTR_UNIQUE;
*** ConstraintElem:
*** 2375,2381
n-indexspace = $6;
$$ = (Node *)n;
}
! | PRIMARY KEY '(' columnList ')' opt_definition OptConsTableSpace
{
Constraint *n = makeNode(Constraint);
n-contype = CONSTR_PRIMARY;
--- 2375,2381
n-indexspace = $6;
$$ = (Node *)n;
}
! | PRIMARY KEY '(' columnList ')' opt_reloptions OptConsTableSpace
{
Constraint *n = makeNode(Constraint);
n-contype = CONSTR_PRIMARY;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] reloptions with a namespace
Alvaro Herrera alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] reloptions with a namespace
Tom Lane escribió: Alvaro Herrera alvhe...@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 Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] reloptions with a namespace
Alvaro Herrera alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] reloptions with a namespace
Tom Lane escribió: Alvaro Herrera alvhe...@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 Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] reloptions with a namespace
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 - 1.24
--- src/backend/access/common/reloptions.c 3 Apr 2009 19:43:35 -
*** transformRelOptions(Datum oldOptions, Li
*** 574,579
--- 574,580
{
ReloptElem*def = lfirst(cell);
+ Assert(IsA(def, ReloptElem));
if (isReset)
{
--
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] reloptions with a namespace
Alvaro Herrera alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] reloptions with a namespace
Alvaro Herrera alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] reloptions with a namespace
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 Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] reloptions with a namespace
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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] reloptions with a namespace
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)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] reloptions with a namespace
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] reloptions with a namespace
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)
Index: src/backend/access/common/reloptions.c
===
RCS file: /home/alvherre/cvs/pgsql/src/backend/access/common/reloptions.c,v
retrieving revision 1.19
diff -c -p -r1.19 reloptions.c
*** src/backend/access/common/reloptions.c 26 Jan 2009 19:41:06 - 1.19
--- src/backend/access/common/reloptions.c 30 Jan 2009 19:42:38 -
***
*** 390,397
}
/*
! * Transform a relation options list (list of DefElem) into the text array
! * format that is kept in pg_class.reloptions.
*
* This is used for three cases: CREATE TABLE/INDEX, ALTER TABLE SET, and
* ALTER TABLE RESET. In the ALTER cases, oldOptions is the existing
--- 390,399
}
/*
! * Transform a relation options list (list of ReloptElem) into the text array
! * format that is kept in pg_class.reloptions, including only those options
! * that are in the passed namespace. The output values do not include the
! * namespace.
*
* This is used for three cases: CREATE TABLE/INDEX, ALTER TABLE SET, and
* ALTER TABLE RESET. In the ALTER cases, oldOptions is the existing
***
*** 402,415
* in the list (it will be or has been handled by interpretOidsOption()).
*
* Note that this is not responsible for determining whether the options
! * are valid.
*
* Both oldOptions and the result are text arrays (or NULL for default),
* but we declare them as Datums to avoid including array.h in reloptions.h.
*/
Datum
! transformRelOptions(Datum oldOptions, List *defList,
! bool ignoreOids, bool isReset)
{
Datum result;
ArrayBuildState *astate;
--- 404,420
* in the list (it will be or has been handled by interpretOidsOption()).
*
* Note that this is not responsible for determining whether the options
! * are valid, but it does check that namespaces for all the options given are
! * listed in validnsps. The NULL namespace is always valid and needs not be
! * explicitely listed. Passing a NULL pointer means that only the NULL
! * namespace is valid.
*
* Both oldOptions and the result are text arrays (or NULL for default),
* but we declare them as Datums to avoid including array.h in reloptions.h.
*/
Datum
! transformRelOptions(Datum oldOptions, List *defList, char *namspace,
! char *validnsps[], bool ignoreOids, bool isReset)
{
Datum result;
ArrayBuildState *astate;
***
*** 444,454
/* Search for a match in defList */
foreach(cell, defList)
{
! DefElem*def = lfirst(cell);
! int kw_len = strlen(def-defname);
if (text_len kw_len text_str[kw_len] == '='
! pg_strncasecmp(text_str, def-defname, kw_len) == 0)
break;
}
if (!cell)
--- 449,471
/* Search for a match in defList */
foreach(cell, defList)
{
! ReloptElem *def = lfirst(cell);
! int kw_len;
+ /* ignore if not in the same namespace */
+ if (namspace == NULL)
+ {
+ if (def-nmspc != NULL)
+ continue;
+ }
+ else if (def-nmspc == NULL)
+ continue;
+ else if (pg_strcasecmp(def-nmspc, namspace) != 0)
+ continue;
+
+ kw_len = strlen(def-optname);
if (text_len kw_len text_str[kw_len] == '='
! pg_strncasecmp(text_str, def-optname, kw_len) == 0)
break;
}
if (!cell)
***
*** 468,474
*/
foreach(cell, defList)
{
! DefElem*def = lfirst(cell);
if (isReset)
{
--- 485,492
*/
foreach(cell, defList)
{
! ReloptElem*def = lfirst(cell);
!
if (isReset)
{
***
*** 483,504
const char *value;
Size len;
! if (ignoreOids pg_strcasecmp(def-defname, oids) == 0)
continue;
/*
! * Flatten the DefElem into a text string like name=arg. If we
! * have just name, assume name=true is meant.
*/
if (def-arg != NULL)
! value = defGetString(def);
else
value = true;
! len = VARHDRSZ +
Re: [HACKERS] reloptions with a namespace
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.0 -0200
--- pgsql.euler/src/backend/access/common/reloptions.c 2009-01-31
02:16:29.0 -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)
+ {
+ boolvalid = 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)
{
Re: [HACKERS] reloptions with a namespace
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.0 -0200
--- pgsql.euler/src/backend/commands/tablecmds.c2009-01-31
01:47:32.0 -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];
boolrepl_null[Natts_pg_class];
boolrepl_repl[Natts_pg_class];
! char *validnsps[] = { toast };
if (defList == NIL)
return; /* nothing to do */
--- 6459,6465
Datum repl_val[Natts_pg_class];
boolrepl_null[Natts_pg_class];
boolrepl_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.c2009-01-31
02:01:22.0 -0200
--- pgsql.euler/src/backend/executor/execMain.c 2009-01-31 01:48:19.0
-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.0
-0200
--- pgsql.euler/src/backend/tcop/utility.c 2009-01-31 01:47:51.0
-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 */
Re: [HACKERS] reloptions with a namespace
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 Herrerahttp://www.advogato.org/person/alvherre
No es bueno caminar con un hombre muerto
Index: src/backend/access/common/reloptions.c
===
RCS file: /home/alvherre/cvs/pgsql/src/backend/access/common/reloptions.c,v
retrieving revision 1.19
diff -c -p -r1.19 reloptions.c
*** src/backend/access/common/reloptions.c 26 Jan 2009 19:41:06 - 1.19
--- src/backend/access/common/reloptions.c 29 Jan 2009 18:01:12 -
***
*** 390,397
}
/*
! * Transform a relation options list (list of DefElem) into the text array
! * format that is kept in pg_class.reloptions.
*
* This is used for three cases: CREATE TABLE/INDEX, ALTER TABLE SET, and
* ALTER TABLE RESET. In the ALTER cases, oldOptions is the existing
--- 390,399
}
/*
! * Transform a relation options list (list of ReloptElem) into the text array
! * format that is kept in pg_class.reloptions, including only those options
! * that are in the passed namespace. The output values do not include the
! * namespace.
*
* This is used for three cases: CREATE TABLE/INDEX, ALTER TABLE SET, and
* ALTER TABLE RESET. In the ALTER cases, oldOptions is the existing
***
*** 402,415
* in the list (it will be or has been handled by interpretOidsOption()).
*
* Note that this is not responsible for determining whether the options
! * are valid.
*
* Both oldOptions and the result are text arrays (or NULL for default),
* but we declare them as Datums to avoid including array.h in reloptions.h.
*/
Datum
! transformRelOptions(Datum oldOptions, List *defList,
! bool ignoreOids, bool isReset)
{
Datum result;
ArrayBuildState *astate;
--- 404,420
* in the list (it will be or has been handled by interpretOidsOption()).
*
* Note that this is not responsible for determining whether the options
! * are valid, but it does check that namespaces for all the options given are
! * listed in validnsps. The NULL namespace is always valid and needs not be
! * explicitely listed. Passing a NULL pointer means that only the NULL
! * namespace is valid.
*
* Both oldOptions and the result are text arrays (or NULL for default),
* but we declare them as Datums to avoid including array.h in reloptions.h.
*/
Datum
! transformRelOptions(Datum oldOptions, List *defList, char *namspace,
! char *validnsps[], bool ignoreOids, bool isReset)
{
Datum result;
ArrayBuildState *astate;
***
*** 444,454
/* Search for a match in defList */
foreach(cell, defList)
{
! DefElem*def = lfirst(cell);
! int kw_len = strlen(def-defname);
if (text_len kw_len text_str[kw_len] == '='
! pg_strncasecmp(text_str, def-defname, kw_len) == 0)
break;
}
if (!cell)
--- 449,471
/* Search for a match in defList */
foreach(cell, defList)
{
! ReloptElem *def = lfirst(cell);
! int kw_len;
+ /* ignore if not in the same namespace */
+ if (namspace == NULL)
+ {
+ if (def-nmspc != NULL)
+ continue;
+ }
+ else if (def-nmspc == NULL)
+ continue;
+ else if (pg_strcasecmp(def-nmspc, namspace) != 0)
+ continue;
+
+ kw_len = strlen(def-optname);
if (text_len kw_len text_str[kw_len] == '='
! pg_strncasecmp(text_str, def-optname, kw_len) == 0)
break;
}
if (!cell)
***
*** 468,474
*/
foreach(cell, defList)
{
! DefElem*def = lfirst(cell);
if (isReset)
{
--- 485,492
*/
foreach(cell, defList)
{
! ReloptElem*def = lfirst(cell);
!
if (isReset)
{
***
*** 483,504
const char *value;
Size len;
! if (ignoreOids pg_strcasecmp(def-defname, oids) == 0)
continue;
/*
! * Flatten the DefElem into a text string like name=arg. If we
! * have just name, assume name=true is meant.
*/
if (def-arg !=
Re: [HACKERS] reloptions with a namespace
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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] reloptions with a namespace
Tom Lane wrote: Alvaro Herrera alvhe...@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 Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] reloptions with a namespace
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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] reloptions with a namespace
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 Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.
Index: src/backend/access/common/reloptions.c
===
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/access/common/reloptions.c,v
retrieving revision 1.18
diff -c -p -r1.18 reloptions.c
*** src/backend/access/common/reloptions.c 12 Jan 2009 21:02:14 - 1.18
--- src/backend/access/common/reloptions.c 14 Jan 2009 14:32:26 -
*** add_string_reloption(int kind, char *nam
*** 390,397
}
/*
! * Transform a relation options list (list of DefElem) into the text array
! * format that is kept in pg_class.reloptions.
*
* This is used for three cases: CREATE TABLE/INDEX, ALTER TABLE SET, and
* ALTER TABLE RESET. In the ALTER cases, oldOptions is the existing
--- 390,399
}
/*
! * Transform a relation options list (list of ReloptElem) into the text array
! * format that is kept in pg_class.reloptions, including only those options
! * that are in the passed namespace. The output values do not include the
! * namespace.
*
* This is used for three cases: CREATE TABLE/INDEX, ALTER TABLE SET, and
* ALTER TABLE RESET. In the ALTER cases, oldOptions is the existing
*** add_string_reloption(int kind, char *nam
*** 402,414
* in the list (it will be or has been handled by interpretOidsOption()).
*
* Note that this is not responsible for determining whether the options
! * are valid.
*
* Both oldOptions and the result are text arrays (or NULL for default),
* but we declare them as Datums to avoid including array.h in reloptions.h.
*/
Datum
! transformRelOptions(Datum oldOptions, List *defList,
bool ignoreOids, bool isReset)
{
Datum result;
--- 404,416
* in the list (it will be or has been handled by interpretOidsOption()).
*
* Note that this is not responsible for determining whether the options
! * are valid, but it does check that the namespaces given are known.
*
* Both oldOptions and the result are text arrays (or NULL for default),
* but we declare them as Datums to avoid including array.h in reloptions.h.
*/
Datum
! transformRelOptions(Datum oldOptions, List *defList, char *namspace,
bool ignoreOids, bool isReset)
{
Datum result;
*** transformRelOptions(Datum oldOptions, Li
*** 444,454
/* Search for a match in defList */
foreach(cell, defList)
{
! DefElem*def = lfirst(cell);
! int kw_len = strlen(def-defname);
if (text_len kw_len text_str[kw_len] == '='
! pg_strncasecmp(text_str, def-defname, kw_len) == 0)
break;
}
if (!cell)
--- 446,468
/* Search for a match in defList */
foreach(cell, defList)
{
! ReloptElem *def = lfirst(cell);
! int kw_len;
+ /* ignore if not in the same namespace */
+ if (namspace == NULL)
+ {
+ if (def-nmspc != NULL)
+ continue;
+ }
+ else if (def-nmspc == NULL)
+ continue;
+ else if (pg_strcasecmp(def-nmspc, namspace) != 0)
+ continue;
+
+ kw_len = strlen(def-optname);
if (text_len kw_len text_str[kw_len] == '='
! pg_strncasecmp(text_str, def-optname, kw_len) == 0)
break;
}
if (!cell)
*** transformRelOptions(Datum oldOptions, Li
*** 468,474
*/
foreach(cell, defList)
{
! DefElem*def = lfirst(cell);
if (isReset)
{
--- 482,493
*/
foreach(cell, defList)
{
! ReloptElem*def = lfirst(cell);
!
! if (def-nmspc pg_strcasecmp(def-nmspc, toast) != 0)
! ereport(ERROR,
! (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
! errmsg(unrecognized parameter namespace \%s\, def-nmspc)));
if (isReset)
{
*** transformRelOptions(Datum oldOptions, Li
*** 483,504
const char *value;
Size len;
! if (ignoreOids pg_strcasecmp(def-defname, oids) == 0)
continue;
/*
! * Flatten the DefElem into a text string like name=arg. If we
! * have just name, assume name=true is meant.
Re: [HACKERS] reloptions with a namespace
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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] reloptions with a namespace
Alvaro Herrera alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
