-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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: fix the reporting of types in information_schema.columns #28945
sql: fix the reporting of types in information_schema.columns #28945
Conversation
6117925
to
1ee00e9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small nit.
Reviewed 4 of 4 files at r1, 1 of 1 files at r2, 18 of 18 files at r3, 2 of 2 files at r4, 7 of 7 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)
pkg/sql/sqlbase/column_type_properties.go, line 299 at r5 (raw file):
} // date, timestamp, interval, int2vector, oidvector, inet, oid, uuid
This list may rot. Rephrase it and say put in (e.g. date, timestamp, interval, )
It is unneeded; we already have a perfectly useful `TypeName` map in package `oid` which does the same. Release note: None
Prior to this patch, CockroachDB incorrectly placed the "input syntax" of each SQL type in the column `data_type` of `information_schema.columns`. The input syntax is the one reported in SHOW COLUMNS, SHOW CREATE TABLE and other places, and is suitable to reproduce the exact type of at able. In contrast, `information_schema.columns.data_type` is constrained by compatibility with third party tools and PostgreSQL clients. It must report the name of the type like PostgreSQL does, which in turn is constrained by the SQL standard. A text column must be reported as "text" not "string"; a decimal column as "numeric" not "decimal", a float8 column as "double precision" not "float8", and so on. By reporting the wrong string in that column CockroachDB is confusing ORMs, which subsequently decide that the current on-disk type is not the one expected by the app and then initiate a schema change (ALTER COLUMN SET TYPE). This patch corrects this incompatibility by introducing logic that produces the proper information schema names for column types. This is expected to reduce ORM complaints about insufficient support for ALTER COLUMN SET TYPE (but will be tested/evaluated separately). Release note (bug fix): CockroachDB now populates the `data_type` column of `information_schema.columns` like PostgreSQL for compatibility with 3rd party tools and ORMs.
1ee00e9
to
55275f5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)
pkg/sql/sqlbase/column_type_properties.go, line 299 at r5 (raw file):
Previously, BramGruneir (Bram Gruneir) wrote…
This list may rot. Rephrase it and say put in (e.g. date, timestamp, interval, )
Done.
bors r+ |
28945: sql: fix the reporting of types in information_schema.columns r=knz a=knz First commits from #28944 and priors. Forked off #28690. Fixes #27601. Largely addresses the concerns that led to issue #26925. Prior to this patch, CockroachDB incorrectly placed the "input syntax" of each SQL type in the column `data_type` of `information_schema.columns`. The input syntax is the one reported in SHOW COLUMNS, SHOW CREATE TABLE and other places, and is suitable to reproduce the exact type of at able. In contrast, `information_schema.columns.data_type` is constrained by compatibility with third party tools and PostgreSQL clients. It must report the name of the type like PostgreSQL does, which in turn is constrained by the SQL standard. A text column must be reported as "text" not "string"; a decimal column as "numeric" not "decimal", a float8 column as "double precision" not "float8", and so on. By reporting the wrong string in that column CockroachDB is confusing ORMs, which subsequently decide that the current on-disk type is not the one expected by the app and then initiate a schema change (ALTER COLUMN SET TYPE). This patch corrects this incompatibility by introducing logic that produces the proper information schema names for column types. This is expected to reduce ORM complaints about insufficient support for ALTER COLUMN SET TYPE (but will be tested/evaluated separately). Release note (bug fix): CockroachDB now populates the `data_type` column of `information_schema.columns` like PostgreSQL for compatibility with 3rd party tools and ORMs. Co-authored-by: Raphael 'kena' Poss <[email protected]>
Build succeeded |
28949: cli,sql: add another crutch to the handling of column types in `dump` r=knz a=knz All commits but the last part of #28945 and priors. PR forked from #28690. There are major problems in `cockroach dump` described separately in #28948. Part of this is `cockroach dump` trying to reverse-engineer a datum type from the announced column type in `information_schema.columns`, by parsing the type name. Prior to this patch, this parsing was incomplete (it went out of sync with the SQL parser over time, naturally as it was bound to do). This entire approach is flawed, but this is no time to redesign `cockroach dump`. Instead, this patch re-syncs the dump-specific type parser with the general SQL parser. It also ensures this parser is still able to parse pre-2.1 type name aliases. Release note (bug fix): `cockroach dump` is now again better able to operate across multiple CockroachDB versions. Co-authored-by: Raphael 'kena' Poss <[email protected]>
29006: backport-2.1: fix the handling of SQL types r=knz a=knz Backport 1/1 commits from #28937. Backport 1/1 commits from #28942. Backport 17/17 commits from #28943. Backport 1/1 commits from #28944. Backport 1/1 commits from #28945. Backport 1/1 commits from #28949. Backport 2/2 commits from #28950. Backport 1/1 commits from #28951 (#28959). Backport 1/1 commits from #28952 (#28959). Backport 1/1 commits from #28955 (#28959). Backport 1/1 commits from #28959. Backport 1/1 commits from #28959. Backport 1/1 commits from #28690. cc @cockroachdb/release Co-authored-by: Raphael 'kena' Poss <[email protected]>
29006: backport-2.1: fix the handling of SQL types r=knz a=knz Backport 1/1 commits from #28937. Backport 1/1 commits from #28942. Backport 17/17 commits from #28943. Backport 1/1 commits from #28944. Backport 1/1 commits from #28945. Backport 1/1 commits from #28949. Backport 2/2 commits from #28950. Backport 1/1 commits from #28951 (#28959). Backport 1/1 commits from #28952 (#28959). Backport 1/1 commits from #28955 (#28959). Backport 1/1 commits from #28959. Backport 1/1 commits from #28959. Backport 1/1 commits from #28690. cc @cockroachdb/release Co-authored-by: Raphael 'kena' Poss <[email protected]>
First commits from #28944 and priors.
Forked off #28690.
Fixes #27601.
Largely addresses the concerns that led to issue #26925.
Prior to this patch, CockroachDB incorrectly placed the "input syntax"
of each SQL type in the column
data_type
ofinformation_schema.columns
.The input syntax is the one reported in SHOW COLUMNS, SHOW CREATE
TABLE and other places, and is suitable to reproduce the exact type of
at able.
In contrast,
information_schema.columns.data_type
is constrained bycompatibility with third party tools and PostgreSQL clients. It must
report the name of the type like PostgreSQL does, which in turn is
constrained by the SQL standard. A text column must be reported as
"text" not "string"; a decimal column as "numeric" not "decimal", a
float8 column as "double precision" not "float8", and so on.
By reporting the wrong string in that column CockroachDB is confusing
ORMs, which subsequently decide that the current on-disk type is not
the one expected by the app and then initiate a schema change (ALTER
COLUMN SET TYPE).
This patch corrects this incompatibility by introducing logic that
produces the proper information schema names for column types. This is
expected to reduce ORM complaints about insufficient support for ALTER
COLUMN SET TYPE (but will be tested/evaluated separately).
Release note (bug fix): CockroachDB now populates the
data_type
column of
information_schema.columns
like PostgreSQL forcompatibility with 3rd party tools and ORMs.