catalog/tabledesc: add tests for TTL validation and cleanup

This moves more of the TTL validation into ttl.go, and outside of the CrossTableReferences validation. Tests are added for all the TTL validation steps. Release justification: test only change and small refactor Release note: None
cockroachdb · Aug 19, 2022 · 7737e82 · 7737e82
1 parent b28f3c9
commit 7737e82
Show file tree

Hide file tree

Showing 3 changed files with 310 additions and 60 deletions.
diff --git a/pkg/sql/catalog/tabledesc/ttl.go b/pkg/sql/catalog/tabledesc/ttl.go
@@ -15,10 +15,12 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/schemaexpr"
 	"github.com/cockroachdb/cockroach/pkg/sql/parser"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
+	"github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented"
 	"github.com/cockroachdb/errors"
 	"github.com/robfig/cron/v3"
 )
@@ -67,11 +69,13 @@ func ValidateRowLevelTTL(ttl *catpb.RowLevelTTL) error {
 	return nil
 }
 
-// ValidateTTLExpirationExpr validates that the ttl_expiration_expression
-// only references existing columns.
-func ValidateTTLExpirationExpr(
-	desc catalog.TableDescriptor, expirationExpr catpb.Expression,
-) error {
+// ValidateTTLExpirationExpr validates that the ttl_expiration_expression, if
+// any, only references existing columns.
+func ValidateTTLExpirationExpr(desc catalog.TableDescriptor) error {
+	if !desc.HasRowLevelTTL() {
+		return nil
+	}
+	expirationExpr := desc.GetRowLevelTTL().ExpirationExpr
 	if expirationExpr == "" {
 		return nil
 	}
@@ -91,6 +95,53 @@ func ValidateTTLExpirationExpr(
 	return nil
 }
 
+// ValidateTTLExpirationColumn validates that the ttl_expire_after setting, if
+// any, is in a valid state. It requires that the TTLDefaultExpirationColumn
+// exists and has DEFAULT/ON UPDATE clauses.
+func ValidateTTLExpirationColumn(desc catalog.TableDescriptor) error {
+	if !desc.HasRowLevelTTL() {
+		return nil
+	}
+	if !desc.GetRowLevelTTL().HasDurationExpr() {
+		return nil
+	}
+	intervalExpr := desc.GetRowLevelTTL().DurationExpr
+	col, err := desc.FindColumnWithName(colinfo.TTLDefaultExpirationColumnName)
+	if err != nil {
+		return errors.Wrapf(err, "expected column %s", colinfo.TTLDefaultExpirationColumnName)
+	}
+	expectedStr := `current_timestamp():::TIMESTAMPTZ + ` + string(intervalExpr)
+	if col.GetDefaultExpr() != expectedStr {
+		return pgerror.Newf(
+			pgcode.InvalidTableDefinition,
+			"expected DEFAULT expression of %s to be %s",
+			colinfo.TTLDefaultExpirationColumnName,
+			expectedStr,
+		)
+	}
+	if col.GetOnUpdateExpr() != expectedStr {
+		return pgerror.Newf(
+			pgcode.InvalidTableDefinition,
+			"expected ON UPDATE expression of %s to be %s",
+			colinfo.TTLDefaultExpirationColumnName,
+			expectedStr,
+		)
+	}
+
+	// For row-level TTL, only ascending PKs are permitted.
+	pk := desc.GetPrimaryIndex()
+	for i := 0; i < pk.NumKeyColumns(); i++ {
+		dir := pk.GetKeyColumnDirection(i)
+		if dir != catpb.IndexColumn_ASC {
+			return unimplemented.NewWithIssuef(
+				76912,
+				`non-ascending ordering on PRIMARY KEYs are not supported with row-level TTL`,
+			)
+		}
+	}
+	return nil
+}
+
 // ValidateTTLBatchSize validates the batch size of a TTL.
 func ValidateTTLBatchSize(key string, val int64) error {
 	if val <= 0 {

diff --git a/pkg/sql/catalog/tabledesc/validate.go b/pkg/sql/catalog/tabledesc/validate.go
@@ -213,51 +213,8 @@ func (desc *wrapper) ValidateCrossReferences(
 		}
 	}
 
-	// For row-level TTL, only ascending PKs are permitted.
+	// For row-level TTL, no FKs are allowed.
 	if desc.HasRowLevelTTL() {
-		rowLevelTTL := desc.RowLevelTTL
-		if rowLevelTTL.HasDurationExpr() {
-			if col, err := desc.FindColumnWithName(colinfo.TTLDefaultExpirationColumnName); err != nil {
-				vea.Report(errors.Wrapf(err, "expected column %s", colinfo.TTLDefaultExpirationColumnName))
-			} else {
-				intervalExpr := desc.GetRowLevelTTL().DurationExpr
-				expectedStr := `current_timestamp():::TIMESTAMPTZ + ` + string(intervalExpr)
-				if col.GetDefaultExpr() != expectedStr {
-					vea.Report(pgerror.Newf(
-						pgcode.InvalidTableDefinition,
-						"expected DEFAULT expression of %s to be %s",
-						colinfo.TTLDefaultExpirationColumnName,
-						expectedStr,
-					))
-				}
-				if col.GetOnUpdateExpr() != expectedStr {
-					vea.Report(pgerror.Newf(
-						pgcode.InvalidTableDefinition,
-						"expected ON UPDATE expression of %s to be %s",
-						colinfo.TTLDefaultExpirationColumnName,
-						expectedStr,
-					))
-				}
-			}
-		}
-
-		if rowLevelTTL.HasExpirationExpr() {
-			_, err := parser.ParseExpr(string(rowLevelTTL.ExpirationExpr))
-			if err != nil {
-				vea.Report(errors.Wrapf(err, "value of 'ttl_expiration_expression' must be a valid expression"))
-			}
-		}
-
-		pk := desc.GetPrimaryIndex()
-		for i := 0; i < pk.NumKeyColumns(); i++ {
-			dir := pk.GetKeyColumnDirection(i)
-			if dir != catpb.IndexColumn_ASC {
-				vea.Report(unimplemented.NewWithIssuef(
-					76912,
-					`non-ascending ordering on PRIMARY KEYs are not supported`,
-				))
-			}
-		}
 		if len(desc.OutboundFKs) > 0 || len(desc.InboundFKs) > 0 {
 			vea.Report(unimplemented.NewWithIssuef(
 				76407,
@@ -784,15 +741,17 @@ func (desc *wrapper) ValidateSelf(vea catalog.ValidationErrorAccumulator) {
 	})
 
 	vea.Report(ValidateRowLevelTTL(desc.GetRowLevelTTL()))
-	if desc.HasRowLevelTTL() {
-		// ValidateTTLExpirationExpr is called separately from ValidateRowLevelTTL
-		// because it can only be called on an initialized table descriptor.
-		// ValidateRowLevelTTL is also used before the table descriptor is fully
-		// initialized to validate the storage parameters.
-		if err := ValidateTTLExpirationExpr(desc, desc.GetRowLevelTTL().ExpirationExpr); err != nil {
-			vea.Report(err)
-			return
-		}
+	// The remaining validation is called separately from ValidateRowLevelTTL
+	// because it can only be called on an initialized table descriptor.
+	// ValidateRowLevelTTL is also used before the table descriptor is fully
+	// initialized to validate the storage parameters.
+	if err := ValidateTTLExpirationExpr(desc); err != nil {
+		vea.Report(err)
+		return
+	}
+	if err := ValidateTTLExpirationColumn(desc); err != nil {
+		vea.Report(err)
+		return
 	}
 
 	// Validate that there are no column with both a foreign key ON UPDATE and an