Thread: note about syntax for fillfactor patch

I see that the just-applied fillfactor patch has changed the syntax
for CREATE TABLE to replace 

OptWithOids:           WITH OIDS                            { $$ = MUST_HAVE_OIDS; }           | WITHOUT OIDS
           { $$ = MUST_NOT_HAVE_OIDS; }           | /*EMPTY*/                          { $$ = DEFAULT_OIDS; }       ; 
 
with

OptWith:           WITH OIDS                       { $$ = list_make1(defWithOids(true)); }           | WITHOUT OIDS
            { $$ = list_make1(defWithOids(false)); }           | WITH definition               { $$ = $2; }           |
WITHOIDS WITH definition     { $$ = lappend($4, defWithOids(true)); }           | WITHOUT OIDS WITH definition  { $$ =
lappend($4,defWithOids(false)); }           | /*EMPTY*/                     { $$ = NIL; }       ;
 

The latter seems seriously grotty: it forces the user to remember an
arbitrary choice about the order in which the two WITHs can be written,
and using the same WITH keyword to introduce two different things seems
ugly.  However, the implementation is such that OIDS can also be
specified in the "definition" option list:

regression=# create table foot(f1 int) with (oids, fillfactor);
CREATE TABLE
regression=# create table foot0(f1 int) with (oids=0, fillfactor);
CREATE TABLE
regression=# select relname,relhasoids from pg_class order by oid desc limit 2;relname | relhasoids
---------+------------foot0   | ffoot    | t
(2 rows)

regression=#

(hm, I wonder why it's not complaining about the bogus fillfactor
specification...)

I propose that we change the syntax to be
WITH OIDS| WITHOUT OIDS| WITH (definition)| /*EMPTY*/

and say that if you want to specify both OIDS and another option you
have to write "oids" or "oids=false" in the definition list.

This gets rid of the hazard that someone might try to specify
conflicting oids options in the hardwired part of the syntax and
in the definition list, and lets the hardwired syntax be deprecated
and perhaps someday removed.

Any objections?
        regards, tom lane

Tom Lane wrote:
> The latter seems seriously grotty: it forces the user to remember an

Agreed.

> I propose that we change the syntax to be
> 
>     WITH OIDS
>     | WITHOUT OIDS
>     | WITH (definition)
>     | /*EMPTY*/
> 
> and say that if you want to specify both OIDS and another option you
> have to write "oids" or "oids=false" in the definition list.
> 
> This gets rid of the hazard that someone might try to specify
> conflicting oids options in the hardwired part of the syntax and
> in the definition list, and lets the hardwired syntax be deprecated
> and perhaps someday removed.
> 
> Any objections?

Sounds good.

--  Bruce Momjian   bruce@momjian.us EnterpriseDB    http://www.enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +

On Sun, 2006-07-02 at 12:06 -0400, Tom Lane wrote:

> I propose that we change the syntax to be
> 
>     WITH OIDS
>     | WITHOUT OIDS
>     | WITH (definition)
>     | /*EMPTY*/
> 
> and say that if you want to specify both OIDS and another option you
> have to write "oids" or "oids=false" in the definition list.

Agreed. Much cleaner.

--  Simon Riggs EnterpriseDB          http://www.enterprisedb.com

Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I propose that we change the syntax to be
> 
>     WITH OIDS
>     | WITHOUT OIDS
>     | WITH (definition)
>     | /*EMPTY*/
> 
> and say that if you want to specify both OIDS and another option you
> have to write "oids" or "oids=false" in the definition list.

Yeah, it sounds good. However, the syntax "oids=false" is not available
for a limitation of the current parser; it can recognize only numerics or
strings as a value. So oids=0/1 or oids='false'/'true' are ok, but
false/true literals are syntax error.


# CREATE TABLE test1 (i int) WITH (oids=0);
CREATE TABLE
# CREATE TABLE test2 (i int) WITH (oids=false);
ERROR:  syntax error at or near "false"
LINE 1: CREATE TABLE test2 (i int) WITH (oids=false);                                             ^
# CREATE TABLE test3 (i int) with (oids='false');
ERROR:  oids requires a boolean value (*) We can resolve this by adding a T_String handler to defGetBoolean().


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

ITAGAKI Takahiro <itagaki.takahiro@oss.ntt.co.jp> writes:
> # CREATE TABLE test1 (i int) WITH (oids=0);
> CREATE TABLE
> # CREATE TABLE test2 (i int) WITH (oids=false);
> ERROR:  syntax error at or near "false"
> LINE 1: CREATE TABLE test2 (i int) WITH (oids=false);
>                                               ^

Yeah, I noticed that.  I think it's easily fixable though --- the
production for def_arg just needs a bit of extension.
        regards, tom lane