Reviewed 9 of 9 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @rytaft)

pkg/sql/create_index.go, line 379 at r1 (raw file):

				"index element",
				semaCtx,
			)

Do we need to check the type? Not every type can be a column (e.g. Any, Tuple, Unknown). Even if we get an error later, it will be be more cryptic - here we can talk about the expression directly (and maybe suggest adding a cast).

We should add some tests like this. Here are a couple I ran:

create table abc (a int, b int, c int);
create index foo on abc (a, (NULL), b); -- it works and creates a column of "unknown" type!
create index foo2 on abc (a, (row(a,b))); -- cryptic error: unimplemented: column crdb_idx_expr_1 is of type  and thus is not indexable

pkg/sql/create_index.go, line 395 at r1 (raw file):

			// Otherwise, create a new virtual column and add it to the table
			// descriptor.
			// TODO(mgartner): If we can determine the expression will never

[nit] this TODO feels like its more for the optimizer than for this code. I don't know if setting nullable would be right, perhaps the expression can become nullable when columns it depends on are changed to be nullable.

pkg/sql/catalog/schemaexpr/computed_column.go, line 39 at r1 (raw file):

//
// TODO(mgartner): Add unit tests for Validate.
func ValidateComputedColumnExpression(

[nit] the new return should be documented - in particular, that it's only useful when we have an Any type for the column,

pkg/sql/catalog/schemaexpr/computed_column.go, line 44 at r1 (raw file):

	d *tree.ColumnTableDef,
	tn *tree.TableName,
	op string,

[supernit] "computed column" isn't really an "op", maybe rename to "description" or "context"

pkg/sql/logictest/testdata/logic_test/pg_catalog, line 2900 at r1 (raw file):

query TTTTTT
select * from pg_indexes where indexname = 'regression_46450_idx'

[nit] maybe change to SELECT indexdef (that column is what 46450 was about).

Reviewed 9 of 9 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner)

pkg/sql/logictest/testdata/logic_test/expression_index, line 64 at r1 (raw file):

)

statement error volatile functions are not allowed in index element

It would be good to try to match the Postgres error messages as much as possible. For example, the error in Postgres for this case is "ERROR: functions in index expression must be marked IMMUTABLE"

pkg/sql/logictest/testdata/logic_test/expression_index, line 71 at r1 (raw file):

statement error index element expression cannot reference computed columns
CREATE TABLE err (a INT, comp INT AS (a + 10) STORED, INDEX ((comp + 10)))

not sure if we care, but this doesn't cause an error in Postgres

Thanks for the review! Now that the RFC is finalized, I've first gone back to bring CREATE INDEX up to spec in #66620. @RaduBerinde I think I've addressed all of your feedback there. We'll get that merged first, and I'll come back to this PR which should be simpler after a rebase.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @rytaft)

pkg/sql/logictest/testdata/logic_test/expression_index, line 64 at r1 (raw file):

I'm relying on tree.TypeCheck to ensure volatility, which is what we do elsewhere, like for computed columns. I think changing this error message just for expression indexes would be non-trivial, so I'm inclined to push back. But if you feel strongly, I can try to find a clean solution.

For what it's worth, for computed columns we differ from Postgres too:

Postgres:

create table t (a int, b int generated always as (a + random()) stored);
ERROR:  42P17: generation expression is not immutable

CRDB:

create table t (a int, b int as (a + random()) stored);
ERROR: random(): volatile functions are not allowed in computed column

pkg/sql/logictest/testdata/logic_test/expression_index, line 71 at r1 (raw file):

Good point. If preventing cycles is the only reason why computed columns can't reference other computed columns, then we may be able to lift this restriction. Because these expression index computed columns will be inaccessible (they can't be referenced in other expressions), then it is guaranteed that there will not be a cycle.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @rytaft)

pkg/sql/logictest/testdata/logic_test/expression_index, line 64 at r1 (raw file):

Yea, not a big deal if it's non-trivial. But I do think it's worth trying to match the Postgres error messages when possible.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner, @RaduBerinde, and @rytaft)