ttl: finalize for v22.1 release

.github/CODEOWNERS

@@ -46,6 +46,7 @@
 
 /pkg/sql/sessiondata/        @cockroachdb/sql-experience
 /pkg/sql/tests/rsg_test.go   @cockroachdb/sql-experience
+/pkg/sql/ttl                 @cockroachdb/sql-experience
 
 /pkg/ccl/schemachangerccl/   @cockroachdb/sql-schema
 /pkg/sql/catalog/            @cockroachdb/sql-schema

pkg/sql/alter_table.go

@@ -1870,6 +1870,13 @@ func handleTTLStorageParamChange(
 		// Adding a TTL requires adding the automatic column and deferring the TTL
 		// addition to after the column is successfully added.
 		tableDesc.RowLevelTTL = nil
+		if _, err := tableDesc.FindColumnWithName(colinfo.TTLDefaultExpirationColumnName); err == nil {
+			return pgerror.Newf(
+				pgcode.InvalidTableDefinition,
+				"cannot add TTL to table with the %s column already defined",
+				colinfo.TTLDefaultExpirationColumnName,
+			)
+		}
 		col, err := rowLevelTTLAutomaticColumnDef(after)
 		if err != nil {
 			return err

pkg/sql/catalog/descriptor.go

@@ -670,6 +670,8 @@ type TableDescriptor interface {
 	// GetExcludeDataFromBackup returns true if the table's row data is configured
 	// to be excluded during backup.
 	GetExcludeDataFromBackup() bool
+	// GetStorageParams returns a list of storage parameters for the table.
+	GetStorageParams(spaceBetweenEqual bool) []string
 }
 
 // TypeDescriptor will eventually be called typedesc.Descriptor.

pkg/sql/catalog/tabledesc/structured.go

@@ -2523,6 +2523,45 @@ func (desc *wrapper) GetExcludeDataFromBackup() bool {
 	return desc.ExcludeDataFromBackup
 }
 
+// GetStorageParams implements the TableDescriptor interface.
+func (desc *wrapper) GetStorageParams(spaceBetweenEqual bool) []string {
+	var storageParams []string
+	var spacing string
+	if spaceBetweenEqual {
+		spacing = ` `
+	}
+	appendStorageParam := func(key, value string) {
+		storageParams = append(storageParams, key+spacing+`=`+spacing+value)
+	}
+	if ttl := desc.GetRowLevelTTL(); ttl != nil {
+		appendStorageParam(`ttl`, `'on'`)
+		appendStorageParam(`ttl_automatic_column`, `'on'`)
+		appendStorageParam(`ttl_expire_after`, string(ttl.DurationExpr))
+		if bs := ttl.SelectBatchSize; bs != 0 {
+			appendStorageParam(`ttl_select_batch_size`, fmt.Sprintf(`%d`, bs))
+		}
+		if bs := ttl.DeleteBatchSize; bs != 0 {
+			appendStorageParam(`ttl_delete_batch_size`, fmt.Sprintf(`%d`, bs))
+		}
+		if cron := ttl.DeletionCron; cron != "" {
+			appendStorageParam(`ttl_job_cron`, fmt.Sprintf(`'%s'`, cron))
+		}
+		if rc := ttl.RangeConcurrency; rc != 0 {
+			appendStorageParam(`ttl_range_concurrency`, fmt.Sprintf(`%d`, rc))
+		}
+		if rl := ttl.DeleteRateLimit; rl != 0 {
+			appendStorageParam(`ttl_delete_rate_limit`, fmt.Sprintf(`%d`, rl))
+		}
+		if pause := ttl.Pause; pause {
+			appendStorageParam(`ttl_pause`, fmt.Sprintf(`%t`, pause))
+		}
+	}
+	if exclude := desc.GetExcludeDataFromBackup(); exclude {
+		appendStorageParam(`exclude_data_from_backup`, `true`)
+	}
+	return storageParams
+}
+
 // GetMultiRegionEnumDependency returns true if the given table has an "implicit"
 // dependency on the multi-region enum. An implicit dependency exists for
 // REGIONAL BY TABLE table's which are homed in an explicit region

pkg/sql/catalog/tabledesc/validate.go

@@ -186,14 +186,51 @@ func (desc *wrapper) ValidateCrossReferences(
 		vea.Report(desc.validateInboundTableRef(by, vdg))
 	}
 
-	// Check foreign keys.
-	if desc.HasRowLevelTTL() && (len(desc.OutboundFKs) > 0 || len(desc.InboundFKs) > 0) {
-		vea.Report(unimplemented.NewWithIssuef(
-			76407,
-			`foreign keys to/from table with TTL "%s" are not permitted`,
-			desc.Name,
-		))
+	// For row-level TTL, only ascending PKs are permitted.
+	if desc.HasRowLevelTTL() {
+		pk := desc.GetPrimaryIndex()
+		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,
+				))
+			}
+		}
+
+		for i := 0; i < pk.NumKeyColumns(); i++ {
+			dir := pk.GetKeyColumnDirection(i)
+			if dir != descpb.IndexDescriptor_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,
+				`foreign keys to/from table with TTL "%s" are not permitted`,
+				desc.Name,
+			))
+		}
 	}
+
+	// Check foreign keys.
 	for i := range desc.OutboundFKs {
 		vea.Report(desc.validateOutboundFK(&desc.OutboundFKs[i], vdg))
 	}

pkg/sql/create_table.go

@@ -1457,7 +1457,6 @@ func NewTableDesc(
 							types.TimestampTZ.SQLString(),
 						)
 					}
-					// TODO(#75428): decide whether we need DefaultExpr/UpdateExpr to match.
 					hasRowLevelTTLColumn = true
 					break
 				}

pkg/sql/logictest/testdata/logic_test/row_level_ttl

@@ -10,6 +10,22 @@ CREATE TABLE tbl (id INT PRIMARY KEY, text TEXT) WITH (ttl = 'on')
 statement error "ttl_expire_after" must be set if "ttl_automatic_column" is set
 CREATE TABLE tbl (id INT PRIMARY KEY, text TEXT) WITH (ttl_automatic_column = 'on')
 
+statement error expected DEFAULT expression of crdb_internal_expiration to be current_timestamp\(\):::TIMESTAMPTZ \+ '00:10:00':::INTERVAL
+CREATE TABLE tbl (
+  id INT PRIMARY KEY,
+  text TEXT,
+  crdb_internal_expiration TIMESTAMPTZ,
+  FAMILY (id, text)
+) WITH (ttl_expire_after = '10 minutes')
+
+statement error expected ON UPDATE expression of crdb_internal_expiration to be current_timestamp\(\):::TIMESTAMPTZ \+ '00:10:00':::INTERVAL
+CREATE TABLE tbl (
+  id INT PRIMARY KEY,
+  text TEXT,
+  crdb_internal_expiration TIMESTAMPTZ DEFAULT current_timestamp() + '10 minutes'::interval,
+  FAMILY (id, text)
+) WITH (ttl_expire_after = '10 minutes')
+
 statement ok
 CREATE TABLE tbl (
   id INT PRIMARY KEY,
@@ -28,16 +44,22 @@ CREATE TABLE public.tbl (
                                                                                             FAMILY fam_0_id_text_crdb_internal_expiration (id, text, crdb_internal_expiration)
 ) WITH (ttl = 'on', ttl_automatic_column = 'on', ttl_expire_after = '00:10:00':::INTERVAL)
 
-statement error resetting "ttl_expire_after" is not permitted\nHINT: use `RESET \(ttl_automatic_column\)` to remove the automatic column or use `RESET \(ttl\)` to remove TTL from the table
+statement error resetting "ttl_expire_after" is not permitted\nHINT: use `RESET \(ttl\)` to remove TTL from the table
 ALTER TABLE tbl RESET (ttl_expire_after)
 
+statement error expected DEFAULT expression of crdb_internal_expiration to be current_timestamp\(\):::TIMESTAMPTZ \+ '00:10:00':::INTERVAL
+ALTER TABLE tbl ALTER COLUMN crdb_internal_expiration SET DEFAULT current_timestamp()
+
+statement error expected ON UPDATE expression of crdb_internal_expiration to be current_timestamp\(\):::TIMESTAMPTZ \+ '00:10:00':::INTERVAL
+ALTER TABLE tbl ALTER COLUMN crdb_internal_expiration SET ON UPDATE current_timestamp()
+
 statement error cannot drop column crdb_internal_expiration while row-level TTL is active
 ALTER TABLE tbl DROP COLUMN crdb_internal_expiration
 
 query T
 SELECT reloptions FROM pg_class WHERE relname = 'tbl'
 ----
-{ttl_expire_after='00:10:00':::INTERVAL}
+{ttl='on',ttl_automatic_column='on',ttl_expire_after='00:10:00':::INTERVAL}
 
 query I
 SELECT count(1) FROM [SHOW SCHEDULES]
@@ -225,7 +247,7 @@ WHERE label = 'row-level-ttl-$table_id'
 query T
 SELECT create_statement FROM [SHOW CREATE SCHEDULE $schedule_id]
 ----
-ALTER TABLE [120 as T] WITH (expire_after = ...)
+ALTER TABLE [122 as T] WITH (expire_after = ...)
 
 statement ok
 DROP TABLE tbl
@@ -244,11 +266,11 @@ query T
 SELECT create_statement FROM [SHOW CREATE TABLE tbl]
 ----
 CREATE TABLE public.tbl (
-                                                                                                                              id INT8 NOT NULL,
-                                                                                                                              text STRING NULL,
-                                                                                                                              crdb_internal_expiration TIMESTAMPTZ NOT VISIBLE NOT NULL DEFAULT current_timestamp():::TIMESTAMPTZ + '00:10:00':::INTERVAL ON UPDATE current_timestamp():::TIMESTAMPTZ + '00:10:00':::INTERVAL,
-                                                                                                                              CONSTRAINT tbl_pkey PRIMARY KEY (id ASC),
-                                                                                                                              FAMILY fam_0_id_text_crdb_internal_expiration (id, text, crdb_internal_expiration)
+                                                                                                                     id INT8 NOT NULL,
+                                                                                                                     text STRING NULL,
+                                                                                                                     crdb_internal_expiration TIMESTAMPTZ NOT VISIBLE NOT NULL DEFAULT current_timestamp():::TIMESTAMPTZ + '00:10:00':::INTERVAL ON UPDATE current_timestamp():::TIMESTAMPTZ + '00:10:00':::INTERVAL,
+                                                                                                                     CONSTRAINT tbl_pkey PRIMARY KEY (id ASC),
+                                                                                                                     FAMILY fam_0_id_text_crdb_internal_expiration (id, text, crdb_internal_expiration)
 ) WITH (ttl = 'on', ttl_automatic_column = 'on', ttl_expire_after = '00:10:00':::INTERVAL, ttl_job_cron = '@daily')
 
 let $table_id
@@ -326,6 +348,11 @@ CREATE TABLE tbl (
   FAMILY (id, text)
 ) WITH (ttl_expire_after = '10 minutes', ttl_select_batch_size = 50, ttl_range_concurrency = 2, ttl_delete_rate_limit = 100, ttl_pause = true)
 
+query T
+SELECT reloptions FROM pg_class WHERE relname = 'tbl'
+----
+{ttl='on',ttl_automatic_column='on',ttl_expire_after='00:10:00':::INTERVAL,ttl_select_batch_size=50,ttl_range_concurrency=2,ttl_delete_rate_limit=100,ttl_pause=true}
+
 query T
 SELECT create_statement FROM [SHOW CREATE TABLE tbl]
 ----
@@ -377,6 +404,19 @@ CREATE TABLE public.tbl (
                                                                                             FAMILY fam_0_id_text_crdb_internal_expiration (id, text, crdb_internal_expiration)
 ) WITH (ttl = 'on', ttl_automatic_column = 'on', ttl_expire_after = '00:10:00':::INTERVAL)
 
+# Test adding to TTL table with crdb_internal_expiration already defined.
+statement ok
+DROP TABLE tbl;
+CREATE TABLE tbl (
+  id INT PRIMARY KEY,
+  text TEXT,
+  crdb_internal_expiration TIMESTAMPTZ,
+  FAMILY (id, text)
+)
+
+statement error cannot add TTL to table with the crdb_internal_expiration column already defined
+ALTER TABLE tbl SET (ttl_expire_after = '10 minutes')
+
 # Test we cannot add FKs to a TTL table.
 statement ok
 CREATE TABLE ref_table (id INT PRIMARY KEY, ref INT)
@@ -402,6 +442,27 @@ CREATE TABLE ttl_become_table (id INT PRIMARY KEY, ref INT REFERENCES ref_table
 statement error foreign keys to/from table with TTL "ttl_become_table" are not permitted
 ALTER TABLE ttl_become_table SET (ttl_expire_after = '10 minutes')
 
+# Check non-ascending PKs are not permitted.
+
+statement ok
+DROP TABLE tbl
+
+statement error non-ascending ordering on PRIMARY KEYs are not supported
+CREATE TABLE tbl (id INT, text TEXT, PRIMARY KEY (id, text DESC)) WITH (ttl_expire_after = '10 minutes')
+
+statement ok
+CREATE TABLE tbl (id INT, text TEXT, PRIMARY KEY (id, text DESC))
+
+statement error non-ascending ordering on PRIMARY KEYs are not supported
+ALTER TABLE tbl SET (ttl_expire_after = '10 minutes')
+
+statement ok
+DROP TABLE tbl;
+CREATE TABLE tbl (id INT, text TEXT, PRIMARY KEY (id, text)) WITH (ttl_expire_after = '10 minutes')
+
+statement error non-ascending ordering on PRIMARY KEYs are not supported
+ALTER TABLE tbl ALTER PRIMARY KEY USING COLUMNS (id, text DESC)
+
 # Create a table without a TTL. Add the TTL to the table and ensure
 # the schedule and TTL is setup correctly.
 statement ok
@@ -423,7 +484,6 @@ ALTER TABLE tbl RESET (ttl_select_batch_size)
 statement ok
 ROLLBACK
 
-
 statement error cannot modify TTL settings while another schema change on the table is being processed
 BEGIN;
 CREATE INDEX tbl_idx ON tbl (text);

pkg/sql/paramparse/paramobserver.go

@@ -208,12 +208,12 @@ var tableParams = map[string]tableParam{
 				po.setAutomaticColumn = true
 			}
 			if !setTrue && po.tableDesc.RowLevelTTL != nil {
-				return unimplemented.NewWithIssue(75428, "unsetting TTL automatic column not yet implemented")
+				return unimplemented.NewWithIssue(76916, "unsetting TTL automatic column not yet implemented")
 			}
 			return nil
 		},
 		onReset: func(po *TableStorageParamObserver, evalCtx *tree.EvalContext, key string) error {
-			return unimplemented.NewWithIssue(75428, "unsetting TTL automatic column not yet implemented")
+			return unimplemented.NewWithIssue(76916, "unsetting TTL automatic column not yet implemented")
 		},
 	},
 	`ttl_expire_after`: {
@@ -258,7 +258,7 @@ var tableParams = map[string]tableParam{
 					pgcode.InvalidParameterValue,
 					`resetting "ttl_expire_after" is not permitted`,
 				),
-				"use `RESET (ttl_automatic_column)` to remove the automatic column or use `RESET (ttl)` to remove TTL from the table",
+				"use `RESET (ttl)` to remove TTL from the table",
 			)
 		},
 	},

pkg/sql/pg_catalog.go

@@ -624,10 +624,12 @@ https://www.postgresql.org/docs/9.5/catalog-pg-class.html`,
 			relPersistence = relPersistenceTemporary
 		}
 		var relOptions tree.Datum = tree.DNull
-		if ttl := table.GetRowLevelTTL(); ttl != nil {
+		if storageParams := table.GetStorageParams(false /* spaceBetweenEqual */); len(storageParams) > 0 {
 			relOptionsArr := tree.NewDArray(types.String)
-			if err := relOptionsArr.Append(tree.NewDString(fmt.Sprintf("ttl_expire_after=%s", ttl.DurationExpr))); err != nil {
-				return err
+			for _, storageParam := range storageParams {
+				if err := relOptionsArr.Append(tree.NewDString(storageParam)); err != nil {
+					return err
+				}
 			}
 			relOptions = relOptionsArr
 		}

pkg/sql/show_create.go

@@ -13,7 +13,6 @@ package sql
 import (
 	"bytes"
 	"context"
-	"fmt"
 	"strings"
 
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
@@ -177,38 +176,7 @@ func ShowCreateTable(
 		return "", err
 	}
 
-	var storageParams []string
-	if ttl := desc.GetRowLevelTTL(); ttl != nil {
-		storageParams = append(
-			storageParams,
-			`ttl = 'on'`,
-			`ttl_automatic_column = 'on'`,
-			fmt.Sprintf(`ttl_expire_after = %s`, ttl.DurationExpr),
-		)
-		if bs := ttl.SelectBatchSize; bs != 0 {
-			storageParams = append(storageParams, fmt.Sprintf(`ttl_select_batch_size = %d`, bs))
-		}
-		if bs := ttl.DeleteBatchSize; bs != 0 {
-			storageParams = append(storageParams, fmt.Sprintf(`ttl_delete_batch_size = %d`, bs))
-		}
-		if cron := ttl.DeletionCron; cron != "" {
-			storageParams = append(storageParams, fmt.Sprintf(`ttl_job_cron = '%s'`, cron))
-		}
-		if rc := ttl.RangeConcurrency; rc != 0 {
-			storageParams = append(storageParams, fmt.Sprintf(`ttl_range_concurrency = %d`, rc))
-		}
-		if rc := ttl.DeleteRateLimit; rc != 0 {
-			storageParams = append(storageParams, fmt.Sprintf(`ttl_delete_rate_limit = %d`, rc))
-		}
-		if pause := ttl.Pause; pause {
-			storageParams = append(storageParams, fmt.Sprintf(`ttl_pause = %t`, pause))
-		}
-	}
-	if exclude := desc.GetExcludeDataFromBackup(); exclude {
-		storageParams = append(storageParams, `exclude_data_from_backup = true`)
-	}
-
-	if len(storageParams) > 0 {
+	if storageParams := desc.GetStorageParams(true /* spaceBetweenEqual */); len(storageParams) > 0 {
 		f.Buffer.WriteString(` WITH (`)
 		f.Buffer.WriteString(strings.Join(storageParams, ", "))
 		f.Buffer.WriteString(`)`)

pkg/sql/ttl/ttljob/ttljob.go

@@ -244,7 +244,6 @@ func (t rowLevelTTLResumer) Resume(ctx context.Context, execCtx interface{}) err
 	var initialVersion descpb.DescriptorVersion
 
 	// TODO(#75428): feature flag check, ttl pause check.
-	// TODO(#75428): only allow ascending order PKs for now schema side.
 	var ttlSettings catpb.RowLevelTTL
 	var pkColumns []string
 	var pkTypes []*types.T
@@ -496,7 +495,8 @@ func runTTLOnRange(
 	ie := execCfg.InternalExecutor
 	db := execCfg.DB
 
-	// TODO(#75428): look at using a dist sql flow job
+	// TODO(#76914): look at using a dist sql flow job, utilize any existing index
+	// on crdb_internal_expiration.
 
 	selectBuilder := makeSelectQueryBuilder(
 		details.TableID,