sql, *: make CockroachDB understand schema names#22753
Merged
knz merged 1 commit intocockroachdb:masterfrom Feb 16, 2018
Merged
sql, *: make CockroachDB understand schema names#22753knz merged 1 commit intocockroachdb:masterfrom
knz merged 1 commit intocockroachdb:masterfrom
Conversation
Member
|
This change is |
528adaf to
b0d2d32
Compare
da47831 to
fa6797e
Compare
"Why":
Prior to this patch, CockroachDB only recognized names of the form
"db.tbl", like MySQL, whereas PostgreSQL compatibility mandates that
"db.public.tbl" be also valid. This needed a change.
**What:**
The visible tip of this iceberg patch is as follows:
- new name resolution functions which are more correct and more easy
to use than the previous code!
Use `Normalize()` on a `TableName`, then `ResolveExistingObject()`
or `ResolveTargetObject()`.
- this in turn relies on generic, well-abstracted name resolution
algorithms in `sem/tree/name_resolution.go`, the definition
of which closely follows the specs in the accompanying RFC.
This handles the pg syntax for catalog and schema names, together
with compatibility rules with CockroachDB v1.x.
- a new schema accessor interface that truly encapsulates schema access!
Check out the gorgeous documentation in `sql/schema_accessors.go`.
In particular:
- `planner` now implements a new interface `SchemaResolver`. Use it.
- if you are not happy with `SchemaResolver` directly, use
`SchemaAccessor` or its various consistuent interfaces.
Depending on whether virtual schemas are to be considered, consider
alternatively `planner.LogicalSchemaAccessor()` and
`planner.PhysicalSchemaAccessor()`.
One may be surprised to see this schema accessor work happen in the
same patch. This was, unfortunately, a requirement to successfully
plug catalog and schema name resolution in all the right places.
Also, it incidentally answers a long-standing demand for a better
interface to access descriptors.
**How:**
The itinerary to achieve this brought me through the following steps:
- the various name resolution algorithms are concentrated in a new
file `sql/sem/tree/name_resolution.go`. They use abstract
interfaces to interface with the "name provider": database, table,
column things coming from the database.
- in the process of writing the name resolution algorithms, I found
that our previous understanding of name resolution was problematic:
- the previous code assumed it was always possible to "qualify a
name using the current database" by just looking at the name
itself and the current database, without any additional
information.
In contrast, the correct qualification algorithms requires both
the current database, the search path, and descriptor
lookups.
This is why this patch fuses all occurrences of separate
"qualification" and "resolution" phases together.
Before: `tn = tn.QualifyWithDatabase(tcurDb); desc = MustGetDesc(tn)`
After: `desc = ResolveExistingObject(p, tn)`
- the resolution code was pushing a `VirtualTabler` very deep in the
lookup routines, with the mistaken understanding that
VirtualTabler is able to respond to requests for database names.
In contrast, VirtualTabler really has nothing to do with database
names, and the corresponding code had to get rid of it.
This was the first motivation for implementing new schema accessor
interfaces.
- the path to decide whether to use cached or non-cached descriptors
was very hard to read; in many instances the code was using
uncached descriptors where cached descriptors would be needed
instead. The various APIs were a mess, and information needed to
decide whether a lookup was possible or not was not available at
the right level(s) of abstraction.
This was the second motivation for implementing new schema accessor
interfaces.
- Once this all was said done, the various consumers of name
resolution had to implement the interfaces. They are:
- resolution of zone specifiers;
- resolution of target names for CCL statements;
- resolution of names (both existing and targets) in the `sql` package;
- some testing code in `sql/opt` and `sql/opt/build`.
- the virtual schemas and tables.
Release note (sql change): CockroachDB now recognizes the syntax
`db.public.tbl` in addition to `db.tbl`, for better compatibility with
PostgreSQL. The handling of the session variable `search_path`, as
well as that of the built-in functions `current_schemas()` and
`current_schema()`, is now closer to that of PostgreSQL.
Release note (sql change): SHOW TABLES FROM can now inspect the tables
of a specific shema, for example `SHOW TABLES FROM db.public` or `SHOW
TABLES FROM db.pg_catalog`.
Release note (sql change): SHOW GRANTS now also shows the schema of
the databases and tables.
fa6797e to
15b73e3
Compare
Contributor
Author
|
Benchmark diffs: |
rytaft
added a commit
to rytaft/cockroach
that referenced
this pull request
Feb 22, 2018
This commit uses the new name resolution code from cockroachdb#22753 to properly resolve names in the optbuilder. Release note: None
This was referenced Feb 22, 2018
nvb
added a commit
to nvb/cockroach
that referenced
this pull request
Apr 30, 2018
Found while working on cockroachdb#22298. This column was missed in cockroachdb#22753. It was displaying each constraint's database instead of each constraint's schema. Release note (bug fix): The constraint_schema column in information_schema.constraint_column_usage now displays the constraint's schema instead of its catalog.
nvb
added a commit
to nvb/cockroach
that referenced
this pull request
May 1, 2018
Found while working on cockroachdb#22298. This column was missed in cockroachdb#22753. It was displaying each constraint's database instead of each constraint's schema. Release note (bug fix): The constraint_schema column in information_schema.constraint_column_usage now displays the constraint's schema instead of its catalog.
craig bot
pushed a commit
that referenced
this pull request
May 1, 2018
25190: sql: fix information_schema.constraint_column_usage.constraint_schema r=nvanbenschoten a=nvanbenschoten Found while working on #22298. This column was missed in #22753. It was displaying each constraint's database instead of each constraint's schema. Release note (bug fix): The constraint_schema column in information_schema.constraint_column_usage now displays the constraint's schema instead of its catalog. 25215: build: make make ignore go files in testdata directories r=knz a=knz Otherwise the special file in pkg/cmd/prereqs/testdata will break. Ideally we'll want to use the 'prereqs' tool instead. For now this will do. Release note: none Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com> Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
craig bot
pushed a commit
that referenced
this pull request
May 2, 2018
25220: backport-2.0: sql: fix information_schema.constraint_column_usage.constraint_schema r=nvanbenschoten a=nvanbenschoten Backport 1/1 commits from #25190. /cc @cockroachdb/release --- Found while working on #22298. This column was missed in #22753. It was displaying each constraint's database instead of each constraint's schema. Release note (bug fix): The constraint_schema column in information_schema.constraint_column_usage now displays the constraint's schema instead of its catalog. Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
andreimatei
added a commit
to andreimatei/cockroach
that referenced
this pull request
Sep 26, 2018
TxnAborter was being used by several tests to modify server test knobs after the server had started. That was racy; one can't simply change a knob from under a running server. The race was introduced in cockroachdb#22753 which needed to defer the activation of the StatementFilter knob. Luckily, since then, the reason for that deferment has gone away: the reason used to be that, if a StatementFilter was installed, we enabled a check in the executor that planning doesn't change the AST. The PR in question was making the planner change the AST by qualifying tables with the public schema. In the executor rewrite, we got rid of that check altogether, so this patch restores the TxnAborter to setting up its knobs before the server starts. Fixes cockroachdb#29028 Release note: None
andreimatei
added a commit
to andreimatei/cockroach
that referenced
this pull request
Sep 26, 2018
TxnAborter was being used by several tests to modify server test knobs after the server had started. That was racy; one can't simply change a knob from under a running server. The race was introduced in cockroachdb#22753 which needed to defer the activation of the StatementFilter knob. Luckily, since then, the reason for that deferment has gone away: the reason used to be that, if a StatementFilter was installed, we enabled a check in the executor that planning doesn't change the AST. The PR in question was making the planner change the AST by qualifying tables with the public schema. In the executor rewrite, we got rid of that check altogether, so this patch restores the TxnAborter to setting up its knobs before the server starts. Fixes cockroachdb#29028 Release note: None
craig bot
pushed a commit
that referenced
this pull request
Sep 26, 2018
30670: sql: fix race in TxnAborter r=andreimatei a=andreimatei TxnAborter was being used by several tests to modify server test knobs after the server had started. That was racy; one can't simply change a knob from under a running server. The race was introduced in #22753 which needed to defer the activation of the StatementFilter knob. Luckily, since then, the reason for that deferment has gone away: the reason used to be that, if a StatementFilter was installed, we enabled a check in the executor that planning doesn't change the AST. The PR in question was making the planner change the AST by qualifying tables with the public schema. In the executor rewrite, we got rid of that check altogether, so this patch restores the TxnAborter to setting up its knobs before the server starts. Fixes #29028 Release note: None Co-authored-by: Andrei Matei <andrei@cockroachlabs.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Result of rebasing #22371 off master.
Fixes #22700.
Implements #21456.
"Why":
Prior to this patch, CockroachDB only recognized names of the form
"db.tbl", like MySQL, whereas PostgreSQL compatibility mandates that
"db.public.tbl" be also valid. This needed a change.
"What"
The visible tip of this iceberg patch is as follows:
new name resolution functions which are more correct and more easy
to use than the previous code!
Use
Normalize()on aTableName, thenResolveExistingObject()or
ResolveTargetObject().this in turn relies on generic, well-abstracted name resolution
algorithms in
sem/tree/name_resolution.go, the definitionof which closely follows the specs in the accompanying RFC (rfcs: proposal to make schemas more pg-compatible #21456).
This handles the pg syntax for catalog and schema names, together
with compatibility rules with CockroachDB v1.x.
a new schema accessor interface that truly encapsulates schema access!
Check out the gorgeous documentation in
sql/schema_accessors.go.In particular:
plannernow implements a new interfaceSchemaResolver. Use it.if you are not happy with
SchemaResolverdirectly, useSchemaAccessoror its various consistuent interfaces.Depending on whether virtual schemas are to be considered, consider
alternatively
planner.LogicalSchemaAccessor()andplanner.PhysicalSchemaAccessor().One may be surprised to see this schema accessor work happen in the
same patch. This was, unfortunately, a requirement to successfully
plug catalog and schema name resolution in all the right places.
Also, it incidentally answers a long-standing demand for a better
interface to access descriptors.
"How"
The itinerary to achieve this brought me through the following steps:
the various name resolution algorithms are concentrated in a new
file
sql/sem/tree/name_resolution.go. They use abstractinterfaces to interface with the "name provider": database, table,
column things coming from the database.
in the process of writing the name resolution algorithms, I found
that our previous understanding of name resolution was problematic:
the previous code assumed it was always possible to "qualify a
name using the current database" by just looking at the name
itself and the current database, without any additional
information.
In contrast, the correct qualification algorithms requires both
the current database, the search path, and descriptor
lookups.
This is why this patch fuses all occurrences of separate
"qualification" and "resolution" phases together.
Before:
tn = tn.QualifyWithDatabase(tcurDb); desc = MustGetDesc(tn)After:
desc = ResolveExistingObject(p, tn)the resolution code was pushing a
VirtualTablervery deep in thelookup routines, with the mistaken understanding that
VirtualTabler is able to respond to requests for database names.
In contrast, VirtualTabler really has nothing to do with database
names, and the corresponding code had to get rid of it.
This was the first motivation for implementing new schema accessor
interfaces.
the path to decide whether to use cached or non-cached descriptors
was very hard to read; in many instances the code was using
uncached descriptors where cached descriptors would be needed
instead. The various APIs were a mess, and information needed to
decide whether a lookup was possible or not was not available at
the right level(s) of abstraction.
This was the second motivation for implementing new schema accessor
interfaces.
Once this all was said done, the various consumers of name
resolution had to implement the interfaces. They are:
sqlpackage;sql/optandsql/opt/build.Release note (sql change): CockroachDB now recognizes the syntax
db.public.tblin addition todb.tbl, for better compatibility withPostgreSQL. The handling of the session variable
search_path, aswell as that of the built-in functions
current_schemas()andcurrent_schema(), is now closer to that of PostgreSQL.Release note (sql change): SHOW TABLES FROM can now inspect the tables
of a specific shema, for example
SHOW TABLES FROM db.publicorSHOW TABLES FROM db.pg_catalog.Release note (sql change): SHOW GRANTS now also shows the schema of
the databases and tables.