Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: incomplete desugaring of type keywords into type names #24686

Closed
knz opened this issue Apr 11, 2018 · 6 comments
Closed

sql: incomplete desugaring of type keywords into type names #24686

knz opened this issue Apr 11, 2018 · 6 comments
Assignees
Labels
A-sql-pgcompat Semantic compatibility with PostgreSQL A-sql-typing SQLtype inference, typing rules, type compatibility. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Milestone

Comments

@knz
Copy link
Contributor

knz commented Apr 11, 2018

Noticed while reviewing cockroachdb/docs#2887.

The following PR #18530 introduced support for the special, internal-only type in PostgreSQL called "char".

This is incomplete, see the comment below for details.

@knz knz changed the title sql: the special internal type `"char" sql: the special internal type "char" is incorrectly defined Apr 11, 2018
@knz knz self-assigned this Apr 11, 2018
@knz knz added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-sql-pgcompat Semantic compatibility with PostgreSQL labels Apr 11, 2018
@jordanlewis
Copy link
Member

I don't really see the problem here - we alias "char" to char because, unlike PG, we don't have an actual behavioral or internal difference between char and "char".

We also now support naming types in quotes, but only the ones that aren't recognized by the SQL standard. This is the same as PG as well. For example:

root@127.0.0.1:56746/demo> create table a (a "int8" primary key);
CREATE TABLE

Time: 6.208098ms
jordan=# create table zxzzz (a "int" primary key);
ERROR:  type "int" does not exist
LINE 1: create table zxzzz (a "int" primary key);
                              ^
jordan=# create table zxzzz (a "int8" primary key);
CREATE TABLE
jordan=#

@knz
Copy link
Contributor Author

knz commented Apr 11, 2018

My point is that the test if $1 == "char" is ineffective.
If you want to make a specially named type called "char", the SQL input syntax for this is """char""" and it will show up after scanning as $1 == "\"char\"".

@jordanlewis
Copy link
Member

No, the input syntax is the INPUT literal "char". Not the string literal "char".

jordan=# create table daaz (a """char""" primary key);
ERROR:  type ""char"" does not exist
LINE 1: create table daaz (a """char""" primary key);

@knz
Copy link
Contributor Author

knz commented Apr 11, 2018

Welp, that is then again a sign that the pg docs are wrong. Thanks for pointing it out.

The situation I am working with is that our docs do not (yet) reflect this properly and the explanation is subtle. I'll need to investigate more before I can have a chat with Lauren and explain what's going on.

@knz knz removed the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Apr 11, 2018
@knz
Copy link
Contributor Author

knz commented Apr 13, 2018

All right I dug into the pg source code and it so happens that both of us were right, although neither of us had the right explanation for it :)

Where I was right:

  • pg applies the same lexical rules and name resolution rules for types as it does for other things. That is, in general (but not for int and char and a few other specific things, see below) lexically the types x and "x" are the same if x is lowercase.
  • pg will recognize any type reachable via search_path with its short name, and no other type beyond that (but see below)
  • this implies (what I suspected from the start), with the standard search path and no user-defined types, pg_catalog.x is the same as x for any predefined types (because they are defined in pg_catalog), that's why pg_catalog.int8 is the same as int8.
  • there is a type pg_catalog.char and that's why it seems legitimate to expect that char and "char" are equivalent input syntax. Yet it doesn't work. Explanation below.

Where you were right:

  • you can't use create table a( a "int") nor create a (a "char") even though create a (a "int8") is valid. See below.

Where we were both missing something:

  • there is no type called int in pg_catalog or anywhere else (!!)
  • there is a grammar rule which desugars the keyword CHAR to the identifier varchar during parsing, even though char is a valid type in pg_catalog.
  • there is a grammar rule which desugars the keyword INT to the identifier int4 during parsing
  • ditto for the following types
Keyword Desugars to
INT int4
INTEGER int4
SMALLINT int2
BIGINT int8
REAL float4
DOUBLE PRECISION float8
DECIMAL numeric
DEC numeric
BOOLEAN bool
BIT bit or varbit depending on whether a precision is specified
[NATIONAL] CHAR varchar or bpchar, depending on whether VARYING is specified
NATIONAL CHARACTER ditto
NCHAR ditto

So the input syntax "char" identifies the type pg_catalog.char because it bypasses the desugaring rule on the CHAR keyword.

@knz knz added this to the 2.1 milestone Apr 13, 2018
@knz knz changed the title sql: the special internal type "char" is incorrectly defined sql: incomplete desugaring of type keywords into type names Apr 13, 2018
@knz knz added A-sql-typing SQLtype inference, typing rules, type compatibility. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. labels May 14, 2018
craig bot pushed a commit that referenced this issue Aug 23, 2018
28690: sql: fix the handling of integer types r=knz a=knz

Addresses a large chunk of #26925.
Fixes #25098.
Informs #24686.

Prior to this patch, CockroachDB maintained an unnecessary distinction
between "INT" and "INTEGER", between "BIGINT" and "INT8", etc.

This distinction is unnecessary but also costly, as we were paying the
price of a "name" attribute in coltypes.TInt, with a string comparison
and hash table lookup on every use of the type.

What really matters is that the type shows up properly in
introspection; this has already been ensured by various
OID-to-pgcatalog mappings and the recently introduced
`InformationSchemaTypeName()`.

Any distinction beyond that is unnecessary and can be dropped from the
implementation.

Release note: None


Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
@knz knz modified the milestones: 2.1, 2.2 Aug 31, 2018
@knz
Copy link
Contributor Author

knz commented Jan 3, 2019

This has been completed (except for NCHAR which is just plainly not supported). Nothing actionable any more here.

@knz knz closed this as completed Jan 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-pgcompat Semantic compatibility with PostgreSQL A-sql-typing SQLtype inference, typing rules, type compatibility. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Development

No branches or pull requests

2 participants