Re: 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.
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.
Attachments:
reloptions-fix-idx.patchtext/x-diff; charset=us-asciiDownload
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 -0000 2.660
--- src/backend/parser/gram.y 3 Apr 2009 19:35:38 -0000
*************** 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;
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