ttl: validate ttl_expiration_expression is a TIMESTAMPTZ

[rafiss]

fixes #86410
fixes #85531
fixes #84725

Previously the only validation is that the expression is valid SQL
syntax. Now the expression must also be a TIMESTAMPTZ which will
prevent errors in the TTL job when comparing to the cutoff.

Validation is also added so that a column cannot be dropped or altered
in type if it is used in the TTL expression. If the column is renamed, the
expression is automatically updated.

Release note: None

Release justification: low risk changes to validation

[cockroach-teamcity]

This change is 

[ajwerner]

Nice, thanks for doing this!

Reviewed 1 of 11 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ecwall and @rafiss)

---

pkg/sql/alter_table.go line 497 at r1 (raw file):

							t.Column,
						),
						"use ALTER TABLE %s RESET (ttl) or ALTER TABLE %s SET (ttl_expiration_expression = ...) instead",

need to repeat the argument or use a positional format specifier

---

pkg/sql/catalog/schemaexpr/expr.go line 461 at r1 (raw file):

// ValidateTTLExpirationExpressionHasNoDependents verifies that the
// ttl_expiration_expression, if any, does not reference the given column.
func ValidateTTLExpirationExpressionHasNoDependents(

this is a weird name given you're validating that the TTLExpression does not depend on the column. Maybe this should be ValidateTTLExpressionDoesNotDependOnColumn?

---

pkg/sql/catalog/tabledesc/validate.go line 786 at r1 (raw file):

	})

	vea.Report(ValidateRowLevelTTL(desc.GetRowLevelTTL()))

Any reason not to add desc as the argument to ValidateRowLevelTTL and call this or add the logic there? It looked to me like all the callers have a descriptor handy.

Code quote:

	vea.Report(ValidateRowLevelTTL(desc.GetRowLevelTTL()))
	if desc.HasRowLevelTTL() {

---

pkg/sql/schemachanger/scbuild/builder_state.go line 473 at r1 (raw file):

		panic(errors.AssertionFailedf("failed to get descriptor for table %d", tableID))
	}
	tableDesc, ok := cachedDesc.desc.(catalog.TableDescriptor)

can you revise this error to say that it's not a table and print the type with %T or DescriptorType()

Code quote:

	tableDesc, ok := cachedDesc.desc.(catalog.TableDescriptor)
	if !ok {
		panic(errors.AssertionFailedf("failed to get descriptor for table %d", tableID))

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_column.go line 78 at r1 (raw file):

	})

	if rowLevelTTL != nil {

nit: invert the condition and early return?

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_column.go line 97 at r1 (raw file):

			}
			wrappedExpr := b.WrapExpression(tbl.TableID, expr)
			for _, referencedCol := range wrappedExpr.ReferencedColumnIDs {

if descpb.ColumnIDs(wrappedExpr.ReferencedColumnIDs).Contains(colToDrop.ColumnID) {?

Code quote:

			for _, referencedCol := range wrappedExpr.ReferencedColumnIDs {
				if referencedCol == colToDrop.ColumnID {

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @ecwall)

---

pkg/sql/alter_table.go line 497 at r1 (raw file):

Previously, ajwerner wrote…

need to repeat the argument or use a positional format specifier

fixing

---

pkg/sql/catalog/schemaexpr/expr.go line 461 at r1 (raw file):

Previously, ajwerner wrote…

this is a weird name given you're validating that the TTLExpression does not depend on the column. Maybe this should be ValidateTTLExpressionDoesNotDependOnColumn?

good call

---

pkg/sql/catalog/tabledesc/validate.go line 786 at r1 (raw file):

Previously, ajwerner wrote…

Any reason not to add desc as the argument to ValidateRowLevelTTL and call this or add the logic there? It looked to me like all the callers have a descriptor handy.

i attempted to explain in the comment below. the reason is that the usage at

cockroach/pkg/sql/storageparam/tablestorageparam/table_storage_param.go

Line 49 in 79ef820

if err := tabledesc.ValidateRowLevelTTL(ttl); err != nil {

can be called with a relatively empty table descriptor when the ttl_expiration_expression param is configured as part of CREATE TABLE

---

pkg/sql/schemachanger/scbuild/builder_state.go line 473 at r1 (raw file):

Previously, ajwerner wrote…

can you revise this error to say that it's not a table and print the type with %T or DescriptorType()

fixing

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_column.go line 78 at r1 (raw file):

Previously, ajwerner wrote…

nit: invert the condition and early return?

fixing

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_column.go line 97 at r1 (raw file):

Previously, ajwerner wrote…

if descpb.ColumnIDs(wrappedExpr.ReferencedColumnIDs).Contains(colToDrop.ColumnID) {?

fixing

[ajwerner]

just needs a rewrite of pkg/sql/schemachanger/scbuild

Reviewed 5 of 11 files at r1, 5 of 8 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @ecwall, and @rafiss)

---

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

	}

	// validate ttl_expiration_expression

nit: fix up this comment.

[postamar]

I appreciate that this PR includes significant coverage in the form of logic tests, still, this seems like a good place to add all of the missing unit test cases in tabledesc for the whole TTL configuration in the table descriptors, as tracked in #86410

[rafiss]

@postamar i've added a separate commit that focuses on TTL validation testing.

[postamar]

Thanks! LGTM

[rafiss]

TFTRs!

bors r=ajwerner,postamar

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build failed:

• Bazel Essential CI (Cockroach)

[rafiss]

bors r=ajwerner,postamar

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)