Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
sql, *: make CockroachDB understand schema names
"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`.
**Details and suggestions for the reviewers:**
The following items are fairly self-contained and can be reviewed in
parallel, and probably should be reviewed first:
1. the changes in `ccl/sqlccl`. This achieves the following two things:
- the search path is plumbed where needed.
- the check for valid table names does not use
`QualifyWithDatabase` any more (because that was removed);
instead they use the new name resolution interface with a shim
that just checks the schema name is valid.
2. the changes in `sql/opt` and `sql/opt/build`. This merely
interfaces with the name resolution algorithm for column items. I
noticed that the underlying data structures there only know about
unqualified table names, so I did not attempt to plumb
catalog/schema name resolution logic in there. No functionality is
lost.
3. the changes in `pkg/config` for zone specifiers. These are
straightforward and merely deal with the correspondance between the
schema part in the CLI zone specifier syntax and the catalog part of
the internal `ZoneSpecifier` struct.
4. the new file `resolver.go`, which contains the entry-point
functions for name resolution using a `SchemaResolver`.
5. the changes to `server/admin.go`. These simply get rid of the
virtual database checks (we have virtual schemas, not databases,
now.)
6. the name resolution algorithms in `sem/tree/name_resolution.go`
and the corresponding tests in `sem/tree/name_resolution_test.go`.
This is where it all started and should be compared to the
specifications in the accompanying RFC.
7. the interface definitions in `schema_accessors.go` and their
implementations in `physical_schema_accessors.go` and
`logical_schema_accessors.go`.
Once the last two items in the list above have been reviewed,
it becomes possible to proceed as follows:
8. check that the various planNode constructors access the new APIs
consistently, with no surprises.
9. check that no harm was done in `table.go` (table cache) and
`database.go` (database cache).
10. check that the construction of the main accessors in
`session.go` make sense.
Suggested review attributions:
- @vivekmenezes: items 7 and 9.
- @petermattis: items 2 and 6.
- @jordanlewis: items 4 and 7.
- @benesch: items 1 and 3.
- @m-schneider: item 8.
- @justinj: item 6.
- @rytaft, @andy-kimball: item 2.
- @BramGruneir, @a-robinson: item 5.
- @andreimatei: item 10.
The following items remain to be done:
- equipping `planner` with the proper interface to become a
`ColumnItemResolver`.
- consequently, fix star expansion.
- upgrading the virtual table definitions as per the RFC.
Release note (sql change): CockroachDB now recognizes the syntax
`db.public.tbl` in addition to `db.tbl`, for better compatibility with
PostgreSQL.- Loading branch information
