new keywords in 9.1

Started by Robert Haasalmost 15 years ago4 messages
#1Robert Haas
robertmhaas@gmail.com
1 attachment(s)

It looks like 9.1 currently introduces 11 new keywords: ATTRIBUTE,
COLLATION, EXTENSION, LABEL, NOREPLICATION, PASSING, REF, REPLICATION,
UNLOGGED, VALIDATE, and XMLEXISTS. Aside from VALIDATE, which is
already being discussed on another thread, are there any of these that
we can/should try to get rid of? At a quick glance, it looks quite
simple to avoid making REPLICATION/NOREPLICATION into keywords, and we
can actually *remove* a bunch of existing keywords using the same
trick. Patch attached.

It would be possible to make CREATE UNLOGGED TABLE work without making
UNLOGGED a keyword using a similar trick, though it's a bit messy.
SELECT .. INTO UNLOGGED foo can't work unless it's a keyword, though,
I think, though I wouldn't cry much if we lost that option. I'm
inclined to think this is not worth messing with more on grounds of
ugliness than anything else. XMLEXISTS is pretty horrible in that the
syntax apparently requires three new keywords (XMLEXISTS, PASSING,
REF) which is pretty lame but I guess it's specified by the standard
so I'm not sure there's much we can do about it. The rest look
reasonable and necessary AFAICT.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Attachments:

role-keywords.patchapplication/octet-stream; name=role-keywords.patchDownload
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 373d2ad..3fa9808 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -489,8 +489,8 @@ static void SplitColQualList(List *qualList,
 	CHARACTER CHARACTERISTICS CHECK CHECKPOINT CLASS CLOSE
 	CLUSTER COALESCE COLLATE COLLATION COLUMN COMMENT COMMENTS COMMIT
 	COMMITTED CONCURRENTLY CONFIGURATION CONNECTION CONSTRAINT CONSTRAINTS
-	CONTENT_P CONTINUE_P CONVERSION_P COPY COST CREATE CREATEDB
-	CREATEROLE CREATEUSER CROSS CSV CURRENT_P
+	CONTENT_P CONTINUE_P CONVERSION_P COPY COST CREATE
+	CROSS CSV CURRENT_P
 	CURRENT_CATALOG CURRENT_DATE CURRENT_ROLE CURRENT_SCHEMA
 	CURRENT_TIME CURRENT_TIMESTAMP CURRENT_USER CURSOR CYCLE
 
@@ -520,13 +520,12 @@ static void SplitColQualList(List *qualList,
 
 	LABEL LANGUAGE LARGE_P LAST_P LC_COLLATE_P LC_CTYPE_P LEADING
 	LEAST LEFT LEVEL LIKE LIMIT LISTEN LOAD LOCAL LOCALTIME LOCALTIMESTAMP
-	LOCATION LOCK_P LOGIN_P
+	LOCATION LOCK_P
 
 	MAPPING MATCH MAXVALUE MINUTE_P MINVALUE MODE MONTH_P MOVE
 
-	NAME_P NAMES NATIONAL NATURAL NCHAR NEXT NO NOCREATEDB
-	NOCREATEROLE NOCREATEUSER NOINHERIT NOLOGIN_P NONE NOREPLICATION_P
-	NOSUPERUSER NOT NOTHING NOTIFY NOTNULL NOWAIT NULL_P NULLIF
+	NAME_P NAMES NATIONAL NATURAL NCHAR NEXT NO NONE
+	NOT NOTHING NOTIFY NOTNULL NOWAIT NULL_P NULLIF
 	NULLS_P NUMERIC
 
 	OBJECT_P OF OFF OFFSET OIDS ON ONLY OPERATOR OPTION OPTIONS OR
@@ -539,14 +538,14 @@ static void SplitColQualList(List *qualList,
 	QUOTE
 
 	RANGE READ REAL REASSIGN RECHECK RECURSIVE REF REFERENCES REINDEX
-	RELATIVE_P RELEASE RENAME REPEATABLE REPLACE REPLICA REPLICATION_P
+	RELATIVE_P RELEASE RENAME REPEATABLE REPLACE REPLICA
 	RESET RESTART RESTRICT RETURNING RETURNS REVOKE RIGHT ROLE ROLLBACK
 	ROW ROWS RULE
 
 	SAVEPOINT SCHEMA SCROLL SEARCH SECOND_P SECURITY SELECT SEQUENCE SEQUENCES
 	SERIALIZABLE SERVER SESSION SESSION_USER SET SETOF SHARE
 	SHOW SIMILAR SIMPLE SMALLINT SOME STABLE STANDALONE_P START STATEMENT
-	STATISTICS STDIN STDOUT STORAGE STRICT_P STRIP_P SUBSTRING SUPERUSER_P
+	STATISTICS STDIN STDOUT STORAGE STRICT_P STRIP_P SUBSTRING
 	SYMMETRIC SYSID SYSTEM_P
 
 	TABLE TABLES TABLESPACE TEMP TEMPLATE TEMPORARY TEXT_P THEN TIME TIMESTAMP
@@ -838,63 +837,10 @@ AlterOptRoleElem:
 					$$ = makeDefElem("unencryptedPassword",
 									 (Node *)makeString($3));
 				}
-			| SUPERUSER_P
-				{
-					$$ = makeDefElem("superuser", (Node *)makeInteger(TRUE));
-				}
-			| NOSUPERUSER
-				{
-					$$ = makeDefElem("superuser", (Node *)makeInteger(FALSE));
-				}
 			| INHERIT
 				{
 					$$ = makeDefElem("inherit", (Node *)makeInteger(TRUE));
 				}
-			| NOINHERIT
-				{
-					$$ = makeDefElem("inherit", (Node *)makeInteger(FALSE));
-				}
-			| CREATEDB
-				{
-					$$ = makeDefElem("createdb", (Node *)makeInteger(TRUE));
-				}
-			| NOCREATEDB
-				{
-					$$ = makeDefElem("createdb", (Node *)makeInteger(FALSE));
-				}
-			| CREATEROLE
-				{
-					$$ = makeDefElem("createrole", (Node *)makeInteger(TRUE));
-				}
-			| NOCREATEROLE
-				{
-					$$ = makeDefElem("createrole", (Node *)makeInteger(FALSE));
-				}
-			| CREATEUSER
-				{
-					/* For backwards compatibility, synonym for SUPERUSER */
-					$$ = makeDefElem("superuser", (Node *)makeInteger(TRUE));
-				}
-			| NOCREATEUSER
-				{
-					$$ = makeDefElem("superuser", (Node *)makeInteger(FALSE));
-				}
-			| LOGIN_P
-				{
-					$$ = makeDefElem("canlogin", (Node *)makeInteger(TRUE));
-				}
-			| NOLOGIN_P
-				{
-					$$ = makeDefElem("canlogin", (Node *)makeInteger(FALSE));
-				}
-			| REPLICATION_P
-				{
-					$$ = makeDefElem("isreplication", (Node *)makeInteger(TRUE));
-				}
-			| NOREPLICATION_P
-				{
-					$$ = makeDefElem("isreplication", (Node *)makeInteger(FALSE));
-				}
 			| CONNECTION LIMIT SignedIconst
 				{
 					$$ = makeDefElem("connectionlimit", (Node *)makeInteger($3));
@@ -908,6 +854,56 @@ AlterOptRoleElem:
 				{
 					$$ = makeDefElem("rolemembers", (Node *)$2);
 				}
+			| IDENT
+				{
+					/*
+					 * We handle identifiers that aren't parser keywords with the following
+					 * special-case codes, to avoid bloating the size of the main parser.
+					 */
+					if (!strcmp($1, "superuser"))
+						$$ = makeDefElem("superuser", (Node *)makeInteger(TRUE));
+					else if (!strcmp($1, "nosuperuser"))
+						$$ = makeDefElem("superuser", (Node *)makeInteger(FALSE));
+					else if (!strcmp($1, "createuser"))
+					{
+						/* For backwards compatibility, synonym for SUPERUSER */
+						$$ = makeDefElem("superuser", (Node *)makeInteger(TRUE));
+					}
+					else if (!strcmp($1, "nocreateuser"))
+					{
+						/* For backwards compatibility, synonym for SUPERUSER */
+						$$ = makeDefElem("superuser", (Node *)makeInteger(FALSE));
+					}
+					else if (!strcmp($1, "createrole"))
+						$$ = makeDefElem("createrole", (Node *)makeInteger(TRUE));
+					else if (!strcmp($1, "nocreaterole"))
+						$$ = makeDefElem("createrole", (Node *)makeInteger(FALSE));
+					else if (!strcmp($1, "replication"))
+						$$ = makeDefElem("isreplication", (Node *)makeInteger(TRUE));
+					else if (!strcmp($1, "noreplication"))
+						$$ = makeDefElem("isreplication", (Node *)makeInteger(FALSE));
+					else if (!strcmp($1, "createdb"))
+						$$ = makeDefElem("createdb", (Node *)makeInteger(TRUE));
+					else if (!strcmp($1, "nocreatedb"))
+						$$ = makeDefElem("createdb", (Node *)makeInteger(FALSE));
+					else if (!strcmp($1, "login"))
+						$$ = makeDefElem("canlogin", (Node *)makeInteger(TRUE));
+					else if (!strcmp($1, "nologin"))
+						$$ = makeDefElem("canlogin", (Node *)makeInteger(FALSE));
+					else if (!strcmp($1, "noinherit"))
+					{
+						/*
+						 * Note that INHERIT is a keyword, so it's handled by main parser, but
+						 * NOINHERIT is handled here.
+						 */
+						$$ = makeDefElem("inherit", (Node *)makeInteger(FALSE));
+					}
+					else
+						ereport(ERROR,
+								(errcode(ERRCODE_SYNTAX_ERROR),
+								 errmsg("unrecognized role option \"%s\"", $1),
+									 parser_errposition(@1)));
+				}
 		;
 
 CreateOptRoleElem:
@@ -11855,9 +11851,6 @@ unreserved_keyword:
 			| CONVERSION_P
 			| COPY
 			| COST
-			| CREATEDB
-			| CREATEROLE
-			| CREATEUSER
 			| CSV
 			| CURRENT_P
 			| CURSOR
@@ -11937,7 +11930,6 @@ unreserved_keyword:
 			| LOCAL
 			| LOCATION
 			| LOCK_P
-			| LOGIN_P
 			| MAPPING
 			| MATCH
 			| MAXVALUE
@@ -11950,13 +11942,6 @@ unreserved_keyword:
 			| NAMES
 			| NEXT
 			| NO
-			| NOCREATEDB
-			| NOCREATEROLE
-			| NOCREATEUSER
-			| NOINHERIT
-			| NOLOGIN_P
-			| NOREPLICATION_P
-			| NOSUPERUSER
 			| NOTHING
 			| NOTIFY
 			| NOWAIT
@@ -11998,7 +11983,6 @@ unreserved_keyword:
 			| REPEATABLE
 			| REPLACE
 			| REPLICA
-			| REPLICATION_P
 			| RESET
 			| RESTART
 			| RESTRICT
@@ -12033,7 +12017,6 @@ unreserved_keyword:
 			| STORAGE
 			| STRICT_P
 			| STRIP_P
-			| SUPERUSER_P
 			| SYSID
 			| SYSTEM_P
 			| TABLES
diff --git a/src/include/parser/kwlist.h b/src/include/parser/kwlist.h
index f288c76..12c2faf 100644
--- a/src/include/parser/kwlist.h
+++ b/src/include/parser/kwlist.h
@@ -96,9 +96,6 @@ PG_KEYWORD("conversion", CONVERSION_P, UNRESERVED_KEYWORD)
 PG_KEYWORD("copy", COPY, UNRESERVED_KEYWORD)
 PG_KEYWORD("cost", COST, UNRESERVED_KEYWORD)
 PG_KEYWORD("create", CREATE, RESERVED_KEYWORD)
-PG_KEYWORD("createdb", CREATEDB, UNRESERVED_KEYWORD)
-PG_KEYWORD("createrole", CREATEROLE, UNRESERVED_KEYWORD)
-PG_KEYWORD("createuser", CREATEUSER, UNRESERVED_KEYWORD)
 PG_KEYWORD("cross", CROSS, TYPE_FUNC_NAME_KEYWORD)
 PG_KEYWORD("csv", CSV, UNRESERVED_KEYWORD)
 PG_KEYWORD("current", CURRENT_P, UNRESERVED_KEYWORD)
@@ -230,7 +227,6 @@ PG_KEYWORD("localtime", LOCALTIME, RESERVED_KEYWORD)
 PG_KEYWORD("localtimestamp", LOCALTIMESTAMP, RESERVED_KEYWORD)
 PG_KEYWORD("location", LOCATION, UNRESERVED_KEYWORD)
 PG_KEYWORD("lock", LOCK_P, UNRESERVED_KEYWORD)
-PG_KEYWORD("login", LOGIN_P, UNRESERVED_KEYWORD)
 PG_KEYWORD("mapping", MAPPING, UNRESERVED_KEYWORD)
 PG_KEYWORD("match", MATCH, UNRESERVED_KEYWORD)
 PG_KEYWORD("maxvalue", MAXVALUE, UNRESERVED_KEYWORD)
@@ -246,14 +242,7 @@ PG_KEYWORD("natural", NATURAL, TYPE_FUNC_NAME_KEYWORD)
 PG_KEYWORD("nchar", NCHAR, COL_NAME_KEYWORD)
 PG_KEYWORD("next", NEXT, UNRESERVED_KEYWORD)
 PG_KEYWORD("no", NO, UNRESERVED_KEYWORD)
-PG_KEYWORD("nocreatedb", NOCREATEDB, UNRESERVED_KEYWORD)
-PG_KEYWORD("nocreaterole", NOCREATEROLE, UNRESERVED_KEYWORD)
-PG_KEYWORD("nocreateuser", NOCREATEUSER, UNRESERVED_KEYWORD)
-PG_KEYWORD("noinherit", NOINHERIT, UNRESERVED_KEYWORD)
-PG_KEYWORD("nologin", NOLOGIN_P, UNRESERVED_KEYWORD)
 PG_KEYWORD("none", NONE, COL_NAME_KEYWORD)
-PG_KEYWORD("noreplication", NOREPLICATION_P, UNRESERVED_KEYWORD)
-PG_KEYWORD("nosuperuser", NOSUPERUSER, UNRESERVED_KEYWORD)
 PG_KEYWORD("not", NOT, RESERVED_KEYWORD)
 PG_KEYWORD("nothing", NOTHING, UNRESERVED_KEYWORD)
 PG_KEYWORD("notify", NOTIFY, UNRESERVED_KEYWORD)
@@ -316,7 +305,6 @@ PG_KEYWORD("rename", RENAME, UNRESERVED_KEYWORD)
 PG_KEYWORD("repeatable", REPEATABLE, UNRESERVED_KEYWORD)
 PG_KEYWORD("replace", REPLACE, UNRESERVED_KEYWORD)
 PG_KEYWORD("replica", REPLICA, UNRESERVED_KEYWORD)
-PG_KEYWORD("replication", REPLICATION_P, UNRESERVED_KEYWORD)
 PG_KEYWORD("reset", RESET, UNRESERVED_KEYWORD)
 PG_KEYWORD("restart", RESTART, UNRESERVED_KEYWORD)
 PG_KEYWORD("restrict", RESTRICT, UNRESERVED_KEYWORD)
@@ -361,7 +349,6 @@ PG_KEYWORD("storage", STORAGE, UNRESERVED_KEYWORD)
 PG_KEYWORD("strict", STRICT_P, UNRESERVED_KEYWORD)
 PG_KEYWORD("strip", STRIP_P, UNRESERVED_KEYWORD)
 PG_KEYWORD("substring", SUBSTRING, COL_NAME_KEYWORD)
-PG_KEYWORD("superuser", SUPERUSER_P, UNRESERVED_KEYWORD)
 PG_KEYWORD("symmetric", SYMMETRIC, RESERVED_KEYWORD)
 PG_KEYWORD("sysid", SYSID, UNRESERVED_KEYWORD)
 PG_KEYWORD("system", SYSTEM_P, UNRESERVED_KEYWORD)
#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#1)
Re: new keywords in 9.1

Robert Haas <robertmhaas@gmail.com> writes:

It looks like 9.1 currently introduces 11 new keywords: ATTRIBUTE,
COLLATION, EXTENSION, LABEL, NOREPLICATION, PASSING, REF, REPLICATION,
UNLOGGED, VALIDATE, and XMLEXISTS. Aside from VALIDATE, which is
already being discussed on another thread, are there any of these that
we can/should try to get rid of? At a quick glance, it looks quite
simple to avoid making REPLICATION/NOREPLICATION into keywords, and we
can actually *remove* a bunch of existing keywords using the same
trick. Patch attached.

One trouble with this trick is that it cannot distinguish between, eg,
SUPERUSER and "superuser" (with the quotes), whereas the latter really
ought not act like a keyword. I'm not sure this is a big enough problem
to justify bloating the parser with extra keywords, though.

It would be possible to make CREATE UNLOGGED TABLE work without making
UNLOGGED a keyword using a similar trick, though it's a bit messy.
SELECT .. INTO UNLOGGED foo can't work unless it's a keyword, though,
I think, though I wouldn't cry much if we lost that option. I'm
inclined to think this is not worth messing with more on grounds of
ugliness than anything else.

-1 for changing that; I think that anything that is used in more than a
very circumscribed context is likely to come back to bite us if we play
cute-looking parser tricks.

A minor stylistic gripe:

+ if (!strcmp($1, "superuser"))

Please spell that as

+ if (strcmp($1, "superuser") == 0)

which is the house style around here. (I have a rant on file in the
archives about exactly why to do that, if you care, but the gist is that
the former way looks like a not-match rather than a match test.)

One other thought about this code is that you could possibly avoid
having gram.y bother with knowledge of the specific keywords at all:
just fling any IDENT into a makeDefElem and let dbcommands.c sort it
out. The syntax error messages might get a bit worse (no error pointer,
in particular) but how much do we care?

regards, tom lane

#3Robert Haas
robertmhaas@gmail.com
In reply to: Tom Lane (#2)
Re: new keywords in 9.1

On Sat, Mar 12, 2011 at 1:36 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

It looks like 9.1 currently introduces 11 new keywords: ATTRIBUTE,
COLLATION, EXTENSION, LABEL, NOREPLICATION, PASSING, REF, REPLICATION,
UNLOGGED, VALIDATE, and XMLEXISTS.  Aside from VALIDATE, which is
already being discussed on another thread, are there any of these that
we can/should try to get rid of?  At a quick glance, it looks quite
simple to avoid making REPLICATION/NOREPLICATION into keywords, and we
can actually *remove* a bunch of existing keywords using the same
trick.  Patch attached.

One trouble with this trick is that it cannot distinguish between, eg,
SUPERUSER and "superuser" (with the quotes), whereas the latter really
ought not act like a keyword.  I'm not sure this is a big enough problem
to justify bloating the parser with extra keywords, though.

I don't think so. We are loosey-goosey about that in other places as
well, e.g. you can do EXPLAIN ("analyze") SELECT ....

It would be possible to make CREATE UNLOGGED TABLE work without making
UNLOGGED a keyword using a similar trick, though it's a bit messy.
SELECT .. INTO UNLOGGED foo can't work unless it's a keyword, though,
I think, though I wouldn't cry much if we lost that option.  I'm
inclined to think this is not worth messing with more on grounds of
ugliness than anything else.

-1 for changing that; I think that anything that is used in more than a
very circumscribed context is likely to come back to bite us if we play
cute-looking parser tricks.

A minor stylistic gripe:

+                                     if (!strcmp($1, "superuser"))

Please spell that as

+                                       if (strcmp($1, "superuser") == 0)

which is the house style around here.  (I have a rant on file in the
archives about exactly why to do that, if you care, but the gist is that
the former way looks like a not-match rather than a match test.)

Well, I think the fact that it is a house style is relevant, but the
reasons are not. To me !strcmp is an idiom that I've seen enough
times that I immediately know what it means, whereas the == 0 style
forces me to stop and scratch my head for a minute to figure out what
the sense of the test is.

One other thought about this code is that you could possibly avoid
having gram.y bother with knowledge of the specific keywords at all:
just fling any IDENT into a makeDefElem and let dbcommands.c sort it
out.  The syntax error messages might get a bit worse (no error pointer,
in particular) but how much do we care?

I can't see any compelling reason to do more work to make the error
messages worse, and frankly I think it makes more sense to have this
code directly in gram.y. It's obviously possible to overdo that
theory, but in this case ISTM it reduces the amount of tracing through
code that must be done to understand how it all works, without really
costing anything.

So, committed, with just the stylistic change mentioned above. Thanks
for the review.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#4Mike Fowler
mike@mlfowler.com
In reply to: Robert Haas (#1)
Re: new keywords in 9.1

On 12/03/11 05:18, Robert Haas wrote:

XMLEXISTS is pretty horrible in that the
syntax apparently requires three new keywords (XMLEXISTS, PASSING,
REF) which is pretty lame but I guess it's specified by the standard
so I'm not sure there's much we can do about it. The rest look
reasonable and necessary AFAICT

I can confirm that I added the three keywords as described in the
SQL/XML standard (section 8.4). Apologies for the delayed confirmation,
I missed the thread when it was started and only noticed when your
commit message arrived.

Regards,

--
Mike Fowler
Registered Linux user: 379787