Skip to content

Commit

Permalink
sql: deprecate ordinal column references
Browse files Browse the repository at this point in the history
This commit disallows ordinal column references by default. Any
statements using them will result in an error. The session setting
`allow_ordinal_column_references` can be set to true to revert to
previous behavior.

Note that numeric tuple indexing (e.g., `SELECT ((1,2,3)).@2`) is still
allowed.

This deprecation is motivated by the inconsistent and surprising
behavior that ordinal column references provide. Ordinal column
references (e.g., `SELECT @1 FROM t`) are a vestigial and undocumented
feature originally motivated to aid the heuristic planner. The PR that
added them, cockroachdb#10729, discourages their use by users because "they are not
robust against schema changes". It also points out a subtle difference
between ordinal column references and SQL standard ordinals that could
confuse users. As an example, the following statements are not
equivalent:

```sql
SELECT @2 FROM t ORDER BY @1;

SELECT @2 FROM t ORDER BY 1;
```

In the current implementation, an ordinal column reference `@n` resolves
to the n-th column in the current scope's slice of columns. This has
several implications:

1. Ordinal columns can refer to any column, not just columns of data
   sources in `FROM` clauses, as was originally intended. This makes it
   hard to reason about the resolution of an ordinal column reference.
   For example, it's somewhat surprising that the result of the `SELECT`
   below is not `(10, 1)`.

```
CREATE TABLE t (a INT, b INT);
INSERT INTO t VALUES (1, 10);

SELECT @2, @1 FROM (SELECT @2, @1 FROM t);
  ?column? | ?column?
-----------+-----------
         1 |       10
(1 row)
```

2. The ordering of columns in the scope's slice is not guaranteed to be
   consistent, so any reasonable ordinal column resolution occurs
   more-or-less by chance. As an example of unexpected behavior,
   consider:

```
CREATE TABLE t (a INT PRIMARY KEY, INDEX ((a+10)));
INSERT INTO t(a) VALUES (1);
ALTER TABLE t ADD COLUMN b INT;

SELECT @1, @2 FROM t;
  ?column? | ?column?
-----------+-----------
         1 |       11
(1 row)
```

Most users would expect the result of the `SELECT` statement to be
`(1, NULL)` because `@1` should resolve to `a` and `@2` should resolve
to `b`. Instead, the ordinal column reference `@2` circumvents logic
that keeps inaccessible columns from being referenced, and resolves to
the virtual computed column backing the secondary index.

Epic: None

Release note (sql change): Ordinal column references (e.g.,
`SELECT @1, @2 FROM t`) are now deprecated. By default, statements using
this syntax will now result in an error. They can be allowed using the
session setting `SET allow_ordinal_column_references=true`. Support for
ordinal column references is scheduled to be removed in version 23.2.
  • Loading branch information
mgartner committed Dec 16, 2022
1 parent 09e338c commit cfa32f1
Show file tree
Hide file tree
Showing 37 changed files with 179 additions and 101 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ db1 sc2 tbl2 table false
db1 sc2 f2 function false

query-sql
SELECT @2 FROM [SHOW CREATE FUNCTION sc1.f1]
SELECT create_statement FROM [SHOW CREATE FUNCTION sc1.f1]
----
CREATE FUNCTION sc1.f1(IN a db1.sc1.enum1)
RETURNS INT8
Expand Down Expand Up @@ -85,7 +85,7 @@ USE db1_new
# 2. db name in qualified name is rewritten.
# 3. sequence id is rewritten so that sequence name is deserialized correctly.
query-sql
SELECT @2 FROM [SHOW CREATE FUNCTION sc1.f1]
SELECT create_statement FROM [SHOW CREATE FUNCTION sc1.f1]
----
CREATE FUNCTION sc1.f1(IN a db1_new.sc1.enum1)
RETURNS INT8
Expand Down Expand Up @@ -200,7 +200,7 @@ db1 sc2 tbl2 table true
db1 sc2 f2 function true

query-sql
SELECT @2 FROM [SHOW CREATE FUNCTION sc1.f1]
SELECT create_statement FROM [SHOW CREATE FUNCTION sc1.f1]
----
CREATE FUNCTION sc1.f1(IN a db1.sc1.enum1)
RETURNS INT8
Expand Down Expand Up @@ -236,7 +236,7 @@ USE db1
# 2. db name in qualified name is rewritten.
# 3. sequence id is rewritten so that sequence name is deserialized correctly.
query-sql
SELECT @2 FROM [SHOW CREATE FUNCTION sc1.f1]
SELECT create_statement FROM [SHOW CREATE FUNCTION sc1.f1]
----
CREATE FUNCTION sc1.f1(IN a db1.sc1.enum1)
RETURNS INT8
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ CREATE TABLE t_to_be_hashed (
);

query T
SELECT @2 FROM [SHOW CREATE TABLE t_to_be_hashed];
SELECT create_statement FROM [SHOW CREATE TABLE t_to_be_hashed];
----
CREATE TABLE public.t_to_be_hashed (
a INT8 NOT NULL,
Expand All @@ -120,7 +120,7 @@ statement ok
CREATE INDEX ON t_to_be_hashed (c) USING HASH;

query T
SELECT @2 FROM [SHOW CREATE TABLE t_to_be_hashed];
SELECT create_statement FROM [SHOW CREATE TABLE t_to_be_hashed];
----
CREATE TABLE public.t_to_be_hashed (
a INT8 NOT NULL,
Expand All @@ -140,7 +140,7 @@ statement ok
CREATE UNIQUE INDEX ON t_to_be_hashed (c) USING HASH;

query T
SELECT @2 FROM [SHOW CREATE TABLE t_to_be_hashed];
SELECT create_statement FROM [SHOW CREATE TABLE t_to_be_hashed];
----
CREATE TABLE public.t_to_be_hashed (
a INT8 NOT NULL,
Expand All @@ -161,7 +161,7 @@ statement ok
ALTER TABLE t_to_be_hashed ALTER PRIMARY KEY USING COLUMNS (a) USING HASH;

query T
SELECT @2 FROM [SHOW CREATE TABLE t_to_be_hashed];
SELECT create_statement FROM [SHOW CREATE TABLE t_to_be_hashed];
----
CREATE TABLE public.t_to_be_hashed (
a INT8 NOT NULL,
Expand Down Expand Up @@ -192,7 +192,7 @@ CREATE TABLE t_idx_pk_hashed_1 (
);

query T
SELECT @2 FROM [SHOW CREATE TABLE t_idx_pk_hashed_1];
SELECT create_statement FROM [SHOW CREATE TABLE t_idx_pk_hashed_1];
----
CREATE TABLE public.t_idx_pk_hashed_1 (
crdb_internal_a_shard_16 INT8 NOT VISIBLE NOT NULL AS (mod(fnv32(crdb_internal.datums_to_bytes(a)), 16:::INT8)) VIRTUAL,
Expand Down Expand Up @@ -223,7 +223,7 @@ CREATE TABLE t_idx_pk_hashed_2 (
);

query T
SELECT @2 FROM [SHOW CREATE TABLE t_idx_pk_hashed_2];
SELECT create_statement FROM [SHOW CREATE TABLE t_idx_pk_hashed_2];
----
CREATE TABLE public.t_idx_pk_hashed_2 (
a INT8 NOT NULL,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ CREATE TABLE t_to_be_hashed (
) LOCALITY REGIONAL BY ROW;

query T
SELECT @2 FROM [SHOW CREATE TABLE t_to_be_hashed];
SELECT create_statement FROM [SHOW CREATE TABLE t_to_be_hashed];
----
CREATE TABLE public.t_to_be_hashed (
pk INT8 NOT NULL,
Expand All @@ -58,7 +58,7 @@ statement ok
CREATE INDEX ON t_to_be_hashed (b) USING HASH;

query T
SELECT @2 FROM [SHOW CREATE TABLE t_to_be_hashed];
SELECT create_statement FROM [SHOW CREATE TABLE t_to_be_hashed];
----
CREATE TABLE public.t_to_be_hashed (
pk INT8 NOT NULL,
Expand All @@ -74,7 +74,7 @@ statement ok
CREATE UNIQUE INDEX ON t_to_be_hashed (b) USING HASH;

query T
SELECT @2 FROM [SHOW CREATE TABLE t_to_be_hashed];
SELECT create_statement FROM [SHOW CREATE TABLE t_to_be_hashed];
----
CREATE TABLE public.t_to_be_hashed (
pk INT8 NOT NULL,
Expand All @@ -91,7 +91,7 @@ statement ok
ALTER TABLE t_to_be_hashed ALTER PRIMARY KEY USING COLUMNS (pk) USING HASH;

query T
SELECT @2 FROM [SHOW CREATE TABLE t_to_be_hashed];
SELECT create_statement FROM [SHOW CREATE TABLE t_to_be_hashed];
----
CREATE TABLE public.t_to_be_hashed (
pk INT8 NOT NULL,
Expand All @@ -114,7 +114,7 @@ CREATE TABLE t_regional_pk_hashed_1 (
) LOCALITY REGIONAL BY ROW;

query T
SELECT @2 FROM [SHOW CREATE TABLE t_regional_pk_hashed_1];
SELECT create_statement FROM [SHOW CREATE TABLE t_regional_pk_hashed_1];
----
CREATE TABLE public.t_regional_pk_hashed_1 (
crdb_internal_pk_shard_16 INT8 NOT VISIBLE NOT NULL AS (mod(fnv32(crdb_internal.datums_to_bytes(pk)), 16:::INT8)) VIRTUAL,
Expand All @@ -137,7 +137,7 @@ CREATE TABLE t_regional_pk_hashed_2 (
) LOCALITY REGIONAL BY ROW;

query T
SELECT @2 FROM [SHOW CREATE TABLE t_regional_pk_hashed_2];
SELECT create_statement FROM [SHOW CREATE TABLE t_regional_pk_hashed_2];
----
CREATE TABLE public.t_regional_pk_hashed_2 (
pk INT8 NOT NULL,
Expand Down
4 changes: 2 additions & 2 deletions pkg/cli/clisqlshell/sql_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ func Example_sql() {
// growing capacity starting at 1 (the batch sizes will be 1, 2, 4, ...),
// and with the query below the division by zero error will occur after the
// first batch consisting of 1 row has been returned to the client.
c.RunWithArgs([]string{`sql`, `-e`, `select 1/(@1-2) from generate_series(1,3)`})
c.RunWithArgs([]string{`sql`, `-e`, `select 1/(i-2) from generate_series(1,3) g(i)`})
c.RunWithArgs([]string{`sql`, `-e`, `SELECT '20:01:02+03:04:05'::timetz AS regression_65066`})
c.RunWithArgs([]string{`sql`, `-e`, `CREATE USER my_user WITH CREATEDB; GRANT admin TO my_user;`})
c.RunWithArgs([]string{`sql`, `-e`, `\du my_user`})
Expand Down Expand Up @@ -116,7 +116,7 @@ func Example_sql() {
// CREATE TABLE
// x
// sql -e copy t.f from stdin
// sql -e select 1/(@1-2) from generate_series(1,3)
// sql -e select 1/(i-2) from generate_series(1,3) g(i)
// ?column?
// -1.0000000000000000000
// (error encountered after some results were delivered)
Expand Down
2 changes: 1 addition & 1 deletion pkg/cli/interactive_tests/test_notice.tcl
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ spawn $argv demo --no-line-editor --no-example-database
eexpect root@

start_test "Test that notices always appear at the end after all results."
send "SELECT IF(@1=4,crdb_internal.notice('hello'),@1) AS MYRES FROM generate_series(1,10);\r"
send "SELECT IF(i=4,crdb_internal.notice('hello'),i) AS MYRES FROM generate_series(1,10) g(i);\r"
eexpect myres
eexpect 1
eexpect 10
Expand Down
12 changes: 6 additions & 6 deletions pkg/cli/interactive_tests/test_url_db_override.tcl
Original file line number Diff line number Diff line change
Expand Up @@ -50,25 +50,25 @@ end_test


start_test "Check that the database flag overrides the db if URL is already set."
spawn $argv sql --url "postgresql://root@localhost:26257/system?sslmode=disable" -e "select length(@1) as l, @1 as db from \[show database\]" --format=csv
spawn $argv sql --url "postgresql://root@localhost:26257/system?sslmode=disable" -e "select length(database) as l, database as db from \[show database\]" --format=csv
eexpect "l,db"
eexpect "6,system"
eexpect eof

spawn $argv sql --url "postgresql://root@localhost:26257/system?sslmode=disable" --database test -e "select length(@1) as l, @1 as db from \[show database\]" --format=csv
spawn $argv sql --url "postgresql://root@localhost:26257/system?sslmode=disable" --database test -e "select length(database) as l, database as db from \[show database\]" --format=csv
eexpect "l,db"
eexpect "4,test"
eexpect eof
end_test

start_test "Check that the database flag does override the database if none was present in the URL."
# Use empty path.
spawn $argv sql --url "postgresql://root@localhost:26257?sslmode=disable" --database system -e "select length(@1) as l, @1 as db from \[show database\]" --format=csv
spawn $argv sql --url "postgresql://root@localhost:26257?sslmode=disable" --database system -e "select length(database) as l, database as db from \[show database\]" --format=csv
eexpect "l,db"
eexpect "6,system"
eexpect eof
# Use path = /
spawn $argv sql --url "postgresql://root@localhost:26257/?sslmode=disable" --database system -e "select length(@1) as l, @1 as db from \[show database\]" --format=csv
spawn $argv sql --url "postgresql://root@localhost:26257/?sslmode=disable" --database system -e "select length(database) as l, database as db from \[show database\]" --format=csv
eexpect "l,db"
eexpect "6,system"
eexpect eof
Expand All @@ -84,12 +84,12 @@ end_test
set ::env(COCKROACH_INSECURE) "false"

start_test "Check that the user flag override the user if URL is already set."
spawn $argv sql --url "postgresql://test@localhost:26257?sslmode=disable" -e "select length(@1) as l, @1 as u from \[show session_user\]" --format=csv
spawn $argv sql --url "postgresql://test@localhost:26257?sslmode=disable" -e "select length(session_user) as l, session_user as u from \[show session_user\]" --format=csv
eexpect "l,u"
eexpect "4,test"
eexpect eof

spawn $argv sql --url "postgresql://root@localhost:26257?sslmode=disable" --user test -e "select length(@1) as l, @1 as u from \[show session_user\]" --format=csv
spawn $argv sql --url "postgresql://root@localhost:26257?sslmode=disable" --user test -e "select length(session_user) as l, session_user as u from \[show session_user\]" --format=csv
eexpect "l,u"
eexpect "4,test"
eexpect eof
Expand Down
8 changes: 4 additions & 4 deletions pkg/sql/delegate/show_syntax.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,17 +27,17 @@ import (
func (d *delegator) delegateShowSyntax(n *tree.ShowSyntax) (tree.Statement, error) {
// Construct an equivalent SELECT query that produces the results:
//
// SELECT @1 AS field, @2 AS message
// SELECT f AS field, m AS message
// FROM (VALUES
// ('file', 'foo.go'),
// ('line', '123'),
// ('function', 'blix()'),
// ('detail', 'some details'),
// ('hint', 'some hints'))
// ('hint', 'some hints')) v(f, m)
//
var query bytes.Buffer
fmt.Fprintf(
&query, "SELECT @1 AS %s, @2 AS %s FROM (VALUES ",
&query, "SELECT f AS %s, m AS %s FROM (VALUES ",
colinfo.ShowSyntaxColumns[0].Name, colinfo.ShowSyntaxColumns[1].Name,
)

Expand All @@ -64,7 +64,7 @@ func (d *delegator) delegateShowSyntax(n *tree.ShowSyntax) (tree.Statement, erro
},
nil, /* reportErr */
)
query.WriteByte(')')
query.WriteString(") v(f, m)")

return parse(query.String())
}
4 changes: 4 additions & 0 deletions pkg/sql/exec_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -3381,6 +3381,10 @@ func (m *sessionDataMutator) SetExperimentalHashGroupJoinEnabled(val bool) {
m.data.ExperimentalHashGroupJoinEnabled = val
}

func (m *sessionDataMutator) SetAllowOrdinalColumnReference(val bool) {
m.data.AllowOrdinalColumnReferences = val
}

// Utility functions related to scrubbing sensitive information on SQL Stats.

// quantizeCounts ensures that the Count field in the
Expand Down
12 changes: 6 additions & 6 deletions pkg/sql/logictest/testdata/logic_test/alter_primary_key
Original file line number Diff line number Diff line change
Expand Up @@ -1394,7 +1394,7 @@ statement ok
ALTER TABLE t ALTER PRIMARY KEY USING COLUMNS (k);

query T
SELECT @2 FROM [SHOW CREATE TABLE t];
SELECT create_statement FROM [SHOW CREATE TABLE t];
----
CREATE TABLE public.t (
a INT8 NOT NULL,
Expand Down Expand Up @@ -1430,7 +1430,7 @@ statement ok
ALTER TABLE t ALTER PRIMARY KEY USING COLUMNS (b, k);

query T
SELECT @2 FROM [SHOW CREATE TABLE t];
SELECT create_statement FROM [SHOW CREATE TABLE t];
----
CREATE TABLE public.t (
a INT8 NOT NULL,
Expand Down Expand Up @@ -1474,7 +1474,7 @@ statement ok
ALTER TABLE t ALTER PRIMARY KEY USING COLUMNS (a);

query T
SELECT @2 FROM [SHOW CREATE TABLE t];
SELECT create_statement FROM [SHOW CREATE TABLE t];
----
CREATE TABLE public.t (
a INT8 NOT NULL,
Expand Down Expand Up @@ -1548,7 +1548,7 @@ statement ok
ALTER TABLE t_test_param ALTER PRIMARY KEY USING COLUMNS (b) USING HASH WITH BUCKET_COUNT = 5;

query T
SELECT @2 FROM [SHOW CREATE TABLE t_test_param]
SELECT create_statement FROM [SHOW CREATE TABLE t_test_param]
----
CREATE TABLE public.t_test_param (
a INT8 NOT NULL,
Expand Down Expand Up @@ -1578,7 +1578,7 @@ COMMENT ON CONSTRAINT i2 ON pkey_comment IS 'idx3';

# Comment should exist inside the create statement, so filter it out
query T
SELECT substring(@2, strpos(@2, 'COMMENT')) FROM [SHOW CREATE pkey_comment];
SELECT substring(create_statement, strpos(create_statement, 'COMMENT')) FROM [SHOW CREATE pkey_comment];
----
COMMENT ON INDEX public.pkey_comment@pkey IS 'idx';
COMMENT ON INDEX public.pkey_comment@i2 IS 'idx2';
Expand All @@ -1592,7 +1592,7 @@ ALTER TABLE pkey_comment ALTER PRIMARY KEY USING COLUMNS (b);
# No comment exists inside the CREATE statement
skipif config local-legacy-schema-changer
query T
SELECT substring(@2, strpos(@2, 'COMMENT')) FROM [SHOW CREATE pkey_comment];
SELECT substring(create_statement, strpos(create_statement, 'COMMENT')) FROM [SHOW CREATE pkey_comment];
----
COMMENT ON INDEX public.pkey_comment@pkey IS 'idx';
COMMENT ON INDEX public.pkey_comment@i2 IS 'idx2';
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/logictest/testdata/logic_test/array
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ a

# From a column ordinal.
query T
SELECT @1[1] FROM (SELECT ARRAY['a','b','c'] AS a)
SELECT a[1] FROM (SELECT ARRAY['a','b','c'] AS a)
----
a

Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/logictest/testdata/logic_test/create_index
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,7 @@ statement ok
CREATE UNIQUE INDEX idx_t_hash_b ON t_hash (b) USING HASH WITH BUCKET_COUNT = 5;

query T
SELECT @2 FROM [SHOW CREATE TABLE t_hash]
SELECT create_statement FROM [SHOW CREATE TABLE t_hash]
----
CREATE TABLE public.t_hash (
pk INT8 NOT NULL,
Expand Down
4 changes: 2 additions & 2 deletions pkg/sql/logictest/testdata/logic_test/create_table
Original file line number Diff line number Diff line change
Expand Up @@ -847,7 +847,7 @@ CREATE TABLE t_good_hash_indexes_1 (
);

query T
SELECT @2 FROM [SHOW CREATE TABLE t_good_hash_indexes_1];
SELECT create_statement FROM [SHOW CREATE TABLE t_good_hash_indexes_1];
----
CREATE TABLE public.t_good_hash_indexes_1 (
crdb_internal_a_shard_5 INT8 NOT VISIBLE NOT NULL AS (mod(fnv32(crdb_internal.datums_to_bytes(a)), 5:::INT8)) VIRTUAL,
Expand All @@ -866,7 +866,7 @@ CREATE TABLE t_good_hash_indexes_2 (
);

query T
SELECT @2 FROM [SHOW CREATE TABLE t_good_hash_indexes_2];
SELECT create_statement FROM [SHOW CREATE TABLE t_good_hash_indexes_2];
----
CREATE TABLE public.t_good_hash_indexes_2 (
a INT8 NOT NULL,
Expand Down
10 changes: 5 additions & 5 deletions pkg/sql/logictest/testdata/logic_test/hash_sharded_index
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# Tests for creating a hash sharded primary key
statement ok
statement ok
CREATE TABLE sharded_primary (a INT PRIMARY KEY USING HASH WITH (bucket_count=10))

query TT
Expand Down Expand Up @@ -769,7 +769,7 @@ statement ok
$create_statement

query T
SELECT @2 FROM [SHOW CREATE TABLE t]
SELECT create_statement FROM [SHOW CREATE TABLE t]
----
CREATE TABLE public.t (
crdb_internal_a_shard_8 INT8 NOT VISIBLE NOT NULL AS (mod(fnv32(crdb_internal.datums_to_bytes(a)), 8:::INT8)) VIRTUAL,
Expand All @@ -792,7 +792,7 @@ CREATE TABLE public.t (
)

query T
SELECT @2 FROM [SHOW CREATE TABLE t]
SELECT create_statement FROM [SHOW CREATE TABLE t]
----
CREATE TABLE public.t (
crdb_internal_a_shard_8 INT4 NOT VISIBLE NOT NULL AS (mod(fnv32(crdb_internal.datums_to_bytes(a)), 8:::INT8)) VIRTUAL,
Expand Down Expand Up @@ -856,7 +856,7 @@ CREATE TABLE t_default_bucket_16 (
);

query T
SELECT @2 FROM [SHOW CREATE TABLE t_default_bucket_16]
SELECT create_statement FROM [SHOW CREATE TABLE t_default_bucket_16]
----
CREATE TABLE public.t_default_bucket_16 (
crdb_internal_a_shard_16 INT8 NOT VISIBLE NOT NULL AS (mod(fnv32(crdb_internal.datums_to_bytes(a)), 16:::INT8)) VIRTUAL,
Expand All @@ -879,7 +879,7 @@ statement ok
CREATE TABLE t_default_bucket_8 (a INT PRIMARY KEY USING HASH);

query T
SELECT @2 FROM [SHOW CREATE TABLE t_default_bucket_8]
SELECT create_statement FROM [SHOW CREATE TABLE t_default_bucket_8]
----
CREATE TABLE public.t_default_bucket_8 (
crdb_internal_a_shard_8 INT8 NOT VISIBLE NOT NULL AS (mod(fnv32(crdb_internal.datums_to_bytes(a)), 8:::INT8)) VIRTUAL,
Expand Down
1 change: 1 addition & 0 deletions pkg/sql/logictest/testdata/logic_test/information_schema
Original file line number Diff line number Diff line change
Expand Up @@ -4739,6 +4739,7 @@ WHERE
);
----
variable value
allow_ordinal_column_references off
allow_prepare_as_opt_plan off
alter_primary_region_super_region_override off
application_name ·
Expand Down

0 comments on commit cfa32f1

Please sign in to comment.