sql: make database, table, view and columns lookups properly case-sensitive #16884

knz · 2017-07-06T15:23:30Z

sql: sanitize the production of error message for tables/views

use the same word "relation" for tables and views for permission
errors and "descriptor does not exist" errors, for consistency with
the existing "already exists" message that already uses the word
"relation".
ensure that names containing special characters are not
doubly quoted in the error message.

sql: make table and view lookups properly case-sensitive

An earlier change introduced pre-normalization of descriptor names
upon descriptor creation, thereby aiming for two goals:

normalize descriptor names upon creation of the descriptor, so as to
avoid the time overhead of re-normalizing the name on every access;
creating a distinction, like one exists in postgres, between
descriptors created with the syntax "A" and the syntax A - the
latter is normalized, the former is not. This makes case sensitivity
opt-in for client applications.

Unfortunately, prior to this patch, most SQL statements also used a
case-insensitive lookup of database/table/view/column names from
storage, preventing the aforementioned benefits.

This patch completes the earlier change by making name lookups
case-sensitive. An exhaustive set of tests is modified/introduced to confirm
that the change is invisible to most common use cases -- i.e. as long
as client apps were not double quoting their identifiers.

Fixes #8862.
Fixes #16858.

cockroach-teamcity · 2017-07-06T15:23:43Z

This change is

knz · 2017-07-06T22:47:36Z

Added the part for column names too.

eisenstatdavid · 2017-07-10T15:32:00Z

Thanks so much for your hard work on this! Aside from some nits (noted below), my main concern is that there are no new/altered tests concerned with Unicode normalization.

Also, has management approved this behavior change?

Reviewed 38 of 48 files at r1, 56 of 56 files at r2.
Review status: all files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.

pkg/ccl/sqlccl/targets.go, line 35 at r2 (raw file):

	starByDatabase := make(map[string]validity, len(targets.Databases))
	for _, d := range targets.Databases {
		starByDatabase[string(d)] = maybeValid

Can you change the type of the map instead of converting everywhere?

pkg/sql/alter_table.go, line 89 at r2 (raw file):

				return err
			}
			_, dropped, err := n.tableDesc.FindColumnByName(string(d.Name))

Could we change the signature of FindColumnByName instead?

pkg/sql/data_source.go, line 677 at r2 (raw file):

		if !ok {
			return nil, nil, fmt.Errorf("relation %q not found",
				parser.AsStringWithFlags(&tableName, parser.FmtBareIdentifiers))

Is this a change that should be made more systematically?

pkg/sql/drop.go, line 433 at r2 (raw file):

			}
			// View does not exist, but we want it to: error out.
			return nil, sqlbase.NewUndefinedTableError(tn)

Perhaps rename to NewUndefinedRelationError?

pkg/sql/table.go, line 388 at r2 (raw file):

			return nil, sqlbase.NewUndefinedTableError(
				&parser.TableName{
					TableName:               parser.Name(fmt.Sprintf("<id=%d>", tableID)),

I vaguely remember that the parser accepts a different syntax for specifying tables by ID? Perhaps I'm confused.

pkg/sql/logictest/testdata/logic_test/case_sensitive_names, line 36 at r2 (raw file):

SHOW CREATE TABLE "A"

statement error relation "test.A" does not exist

Is there a good reason that this one is not like the others?

pkg/sql/logictest/testdata/logic_test/select, line 173 at r2 (raw file):


query T
SELECT "foo"."v" FROM kv AS foo WHERE foo.k = 'a'

Are the quotes still needed?

Comments from Reviewable

knz · 2017-07-14T21:36:29Z

@bdarnell said "ok as long as it brings us closer to pg compatibility".

Review status: 69 of 99 files reviewed at latest revision, 7 unresolved discussions.

pkg/ccl/sqlccl/targets.go, line 35 at r2 (raw file):