Merge #145374

145374: ttljob: avoid using different versions of enum; use a desc.Collection tied to the correct txn r=rafiss a=rafiss

### ttl: use a desc.Collection tied to the txn for TTL job metadata

Previously, we would pass in a desc.Collection from the flowCtx while gathering metadata for the TTL job, such as column names and types.

This was incorrect, since it meant that the lease on any descriptors fetched in those transactions would never be released until the job completed.

###  ttljob: make an error message slightly more helpful

The new message assures the user that the job will run again.

### ttljob: avoid using different versions of enum

This patch changes the SELECT/DELETE queries so the use the logical string
representation of enums, rather than the encoded datum. This avoids an
issue that was caused by the enum descriptor's version being passed in
as part of the query placeholder argument.

fixes #145908
Informs: https://github.com/cockroachlabs/support/issues/3246

Release note (bug fix): Fixed a bug that could cause the row level TTL
job to fail with a "comparison of two different versions of enum" error
if an ENUM type referenced by the table experienced a schema change.



Co-authored-by: Rafi Shamim <[email protected]>

Author: craig[bot]

pkg/sql/schema_changer_test.go

@@ -6911,7 +6911,7 @@ CREATE TABLE t.test (id TEXT PRIMARY KEY, expire_at TIMESTAMPTZ) WITH (ttl_expir
 			desc:  "error during multiple ALTER TABLE x SET ttl_expire_after x during modify row-level-ttl mutation",
 			setup: createNonTTLTable,
 			schemaChange: `
-ALTER TABLE t.test SET (ttl_cron = '@daily');
+ALTER TABLE t.test SET (ttl_job_cron = '@daily');
 ALTER TABLE t.test SET (ttl_expire_after = '10 hours');
 `,
 			runBeforeModifyRowLevelTTL: createFailOnceFunc(),

pkg/sql/sem/tree/datum.go

@@ -5460,7 +5460,7 @@ func (d *DEnum) ResolvedType() *types.T {
 	return d.EnumTyp
 }
 
-// PlanCistFromCtx returns the plan gist if it is stored in the context. It is
+// PlanGistFromCtx returns the plan gist if it is stored in the context. It is
 // injected from the sql package to avoid import cycle.
 var PlanGistFromCtx func(context.Context) string
 
@@ -5487,11 +5487,11 @@ func (d *DEnum) Compare(ctx context.Context, cmpCtx CompareContext, other Datum)
 			// safe string.
 			gist = redact.SafeString(PlanGistFromCtx(ctx))
 		}
-		panic(errors.AssertionFailedf(
+		return 0, errors.AssertionFailedf(
 			"comparison of two different versions of enum %s oid %d: versions %d and %d, gist %q",
 			d.EnumTyp.SQLStringForError(), errors.Safe(d.EnumTyp.Oid()), d.EnumTyp.TypeMeta.Version,
 			v.EnumTyp.TypeMeta.Version, gist,
-		))
+		)
 	}
 
 	res := bytes.Compare(d.PhysicalRep, v.PhysicalRep)

pkg/sql/ttl/ttlbase/BUILD.bazel

@@ -13,6 +13,7 @@ go_library(
         "//pkg/sql/catalog/catenumpb",
         "//pkg/sql/catalog/catpb",
         "//pkg/sql/catalog/tabledesc",
+        "//pkg/sql/types",
         "@com_github_cockroachdb_errors//:errors",
     ],
 )
@@ -23,6 +24,7 @@ go_test(
     embed = [":ttlbase"],
     deps = [
         "//pkg/sql/catalog/catenumpb",
+        "//pkg/sql/types",
         "//pkg/util/leaktest",
         "//pkg/util/log",
         "@com_github_stretchr_testify//require",

pkg/sql/ttl/ttlbase/ttl_helpers.go

@@ -16,6 +16,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catenumpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
+	"github.com/cockroachdb/cockroach/pkg/sql/types"
 	"github.com/cockroachdb/errors"
 )
 
@@ -169,6 +170,7 @@ func BuildSelectQuery(
 	relationName string,
 	pkColNames []string,
 	pkColDirs []catenumpb.IndexColumn_Direction,
+	pkColTypes []*types.T,
 	aostDuration time.Duration,
 	ttlExpr catpb.Expression,
 	numStartQueryBounds, numEndQueryBounds int,
@@ -217,6 +219,8 @@ func BuildSelectQuery(
 					buf.WriteString(pkColNames[j])
 					buf.WriteString(" = $")
 					buf.WriteString(strconv.Itoa(j + placeholderOffset))
+					buf.WriteString("::")
+					buf.WriteString(pkColTypes[j].SQLStringFullyQualified())
 					buf.WriteString(" AND ")
 				}
 				buf.WriteString(pkColNames[i])
@@ -227,6 +231,8 @@ func BuildSelectQuery(
 				}
 				buf.WriteString(" $")
 				buf.WriteString(strconv.Itoa(i + placeholderOffset))
+				buf.WriteString("::")
+				buf.WriteString(pkColTypes[i].SQLStringFullyQualified())
 				buf.WriteString(")")
 				if !isLast {
 					buf.WriteString(" OR")

pkg/sql/ttl/ttlbase/ttl_helpers_test.go

@@ -9,6 +9,7 @@ import (
 	"testing"
 
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catenumpb"
+	"github.com/cockroachdb/cockroach/pkg/sql/types"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/stretchr/testify/require"
@@ -43,10 +44,10 @@ FROM relation_name
 AS OF SYSTEM TIME INTERVAL '-30 seconds'
 WHERE ((expire_at) <= $1)
 AND (
-  (col0 > $3)
+  (col0 > $3::INT8)
 )
 AND (
-  (col0 <= $2)
+  (col0 <= $2::INT8)
 )
 ORDER BY col0 ASC
 LIMIT 2`,
@@ -63,10 +64,10 @@ FROM relation_name
 AS OF SYSTEM TIME INTERVAL '-30 seconds'
 WHERE ((expire_at) <= $1)
 AND (
-  (col0 < $3)
+  (col0 < $3::INT8)
 )
 AND (
-  (col0 >= $2)
+  (col0 >= $2::INT8)
 )
 ORDER BY col0 DESC
 LIMIT 2`,
@@ -113,10 +114,10 @@ FROM relation_name
 AS OF SYSTEM TIME INTERVAL '-30 seconds'
 WHERE ((expire_at) <= $1)
 AND (
-  (col0 >= $3)
+  (col0 >= $3::INT8)
 )
 AND (
-  (col0 <= $2)
+  (col0 <= $2::INT8)
 )
 ORDER BY col0 ASC
 LIMIT 2`,
@@ -134,10 +135,10 @@ FROM relation_name
 AS OF SYSTEM TIME INTERVAL '-30 seconds'
 WHERE ((expire_at) <= $1)
 AND (
-  (col0 <= $3)
+  (col0 <= $3::INT8)
 )
 AND (
-  (col0 >= $2)
+  (col0 >= $2::INT8)
 )
 ORDER BY col0 DESC
 LIMIT 2`,
@@ -155,12 +156,12 @@ FROM relation_name
 AS OF SYSTEM TIME INTERVAL '-30 seconds'
 WHERE ((expire_at) <= $1)
 AND (
-  (col0 > $4) OR
-  (col0 = $4 AND col1 > $5)
+  (col0 > $4::INT8) OR
+  (col0 = $4::INT8 AND col1 > $5::INT8)
 )
 AND (
-  (col0 < $2) OR
-  (col0 = $2 AND col1 <= $3)
+  (col0 < $2::INT8) OR
+  (col0 = $2::INT8 AND col1 <= $3::INT8)
 )
 ORDER BY col0 ASC, col1 ASC
 LIMIT 2`,
@@ -178,11 +179,11 @@ FROM relation_name
 AS OF SYSTEM TIME INTERVAL '-30 seconds'
 WHERE ((expire_at) <= $1)
 AND (
-  (col0 > $4)
+  (col0 > $4::INT8)
 )
 AND (
-  (col0 < $2) OR
-  (col0 = $2 AND col1 <= $3)
+  (col0 < $2::INT8) OR
+  (col0 = $2::INT8 AND col1 <= $3::INT8)
 )
 ORDER BY col0 ASC, col1 ASC
 LIMIT 2`,
@@ -200,11 +201,11 @@ FROM relation_name
 AS OF SYSTEM TIME INTERVAL '-30 seconds'
 WHERE ((expire_at) <= $1)
 AND (
-  (col0 > $3) OR
-  (col0 = $3 AND col1 > $4)
+  (col0 > $3::INT8) OR
+  (col0 = $3::INT8 AND col1 > $4::INT8)
 )
 AND (
-  (col0 <= $2)
+  (col0 <= $2::INT8)
 )
 ORDER BY col0 ASC, col1 ASC
 LIMIT 2`,
@@ -222,12 +223,12 @@ FROM relation_name
 AS OF SYSTEM TIME INTERVAL '-30 seconds'
 WHERE ((expire_at) <= $1)
 AND (
-  (col0 < $4) OR
-  (col0 = $4 AND col1 < $5)
+  (col0 < $4::INT8) OR
+  (col0 = $4::INT8 AND col1 < $5::INT8)
 )
 AND (
-  (col0 > $2) OR
-  (col0 = $2 AND col1 >= $3)
+  (col0 > $2::INT8) OR
+  (col0 = $2::INT8 AND col1 >= $3::INT8)
 )
 ORDER BY col0 DESC, col1 DESC
 LIMIT 2`,
@@ -245,11 +246,11 @@ FROM relation_name
 AS OF SYSTEM TIME INTERVAL '-30 seconds'
 WHERE ((expire_at) <= $1)
 AND (
-  (col0 < $4)
+  (col0 < $4::INT8)
 )
 AND (
-  (col0 > $2) OR
-  (col0 = $2 AND col1 >= $3)
+  (col0 > $2::INT8) OR
+  (col0 = $2::INT8 AND col1 >= $3::INT8)
 )
 ORDER BY col0 DESC, col1 DESC
 LIMIT 2`,
@@ -267,11 +268,11 @@ FROM relation_name
 AS OF SYSTEM TIME INTERVAL '-30 seconds'
 WHERE ((expire_at) <= $1)
 AND (
-  (col0 < $3) OR
-  (col0 = $3 AND col1 < $4)
+  (col0 < $3::INT8) OR
+  (col0 = $3::INT8 AND col1 < $4::INT8)
 )
 AND (
-  (col0 >= $2)
+  (col0 >= $2::INT8)
 )
 ORDER BY col0 DESC, col1 DESC
 LIMIT 2`,
@@ -290,14 +291,14 @@ FROM relation_name
 AS OF SYSTEM TIME INTERVAL '-30 seconds'
 WHERE ((expire_at) <= $1)
 AND (
-  (col0 > $5) OR
-  (col0 = $5 AND col1 < $6) OR
-  (col0 = $5 AND col1 = $6 AND col2 > $7)
+  (col0 > $5::INT8) OR
+  (col0 = $5::INT8 AND col1 < $6::INT8) OR
+  (col0 = $5::INT8 AND col1 = $6::INT8 AND col2 > $7::INT8)
 )
 AND (
-  (col0 < $2) OR
-  (col0 = $2 AND col1 > $3) OR
-  (col0 = $2 AND col1 = $3 AND col2 <= $4)
+  (col0 < $2::INT8) OR
+  (col0 = $2::INT8 AND col1 > $3::INT8) OR
+  (col0 = $2::INT8 AND col1 = $3::INT8 AND col2 <= $4::INT8)
 )
 ORDER BY col0 ASC, col1 DESC, col2 ASC
 LIMIT 2`,
@@ -316,14 +317,14 @@ FROM relation_name
 AS OF SYSTEM TIME INTERVAL '-30 seconds'
 WHERE ((expire_at) <= $1)
 AND (
-  (col0 < $5) OR
-  (col0 = $5 AND col1 > $6) OR
-  (col0 = $5 AND col1 = $6 AND col2 < $7)
+  (col0 < $5::INT8) OR
+  (col0 = $5::INT8 AND col1 > $6::INT8) OR
+  (col0 = $5::INT8 AND col1 = $6::INT8 AND col2 < $7::INT8)
 )
 AND (
-  (col0 > $2) OR
-  (col0 = $2 AND col1 < $3) OR
-  (col0 = $2 AND col1 = $3 AND col2 >= $4)
+  (col0 > $2::INT8) OR
+  (col0 = $2::INT8 AND col1 < $3::INT8) OR
+  (col0 = $2::INT8 AND col1 = $3::INT8 AND col2 >= $4::INT8)
 )
 ORDER BY col0 DESC, col1 ASC, col2 DESC
 LIMIT 2`,
@@ -334,10 +335,15 @@ LIMIT 2`,
 		t.Run(tc.desc, func(t *testing.T) {
 			pkColDirs := tc.pkColDirs
 			pkColNames := GenPKColNames(len(pkColDirs))
+			pkColTypes := make([]*types.T, len(pkColDirs))
+			for i := range pkColDirs {
+				pkColTypes[i] = types.Int
+			}
 			actualQuery := BuildSelectQuery(
 				relationName,
 				pkColNames,
 				pkColDirs,
+				pkColTypes,
 				DefaultAOSTDuration,
 				ttlExpr,
 				tc.numStartQueryBounds,

pkg/sql/ttl/ttljob/BUILD.bazel

@@ -91,7 +91,6 @@ go_test(
         "//pkg/sql/catalog",
         "//pkg/sql/catalog/catenumpb",
         "//pkg/sql/catalog/catpb",
-        "//pkg/sql/catalog/catsessiondata",
         "//pkg/sql/catalog/colinfo",
         "//pkg/sql/catalog/descpb",
         "//pkg/sql/catalog/descs",
@@ -105,7 +104,6 @@ go_test(
         "//pkg/sql/rowenc",
         "//pkg/sql/sem/eval",
         "//pkg/sql/sem/tree",
-        "//pkg/sql/sessiondata",
         "//pkg/sql/ttl/ttlbase",
         "//pkg/sql/types",
         "//pkg/testutils",

pkg/sql/ttl/ttljob/testdata/ttljob_plans

@@ -18,10 +18,10 @@ FROM defaultdb.public.p@"p_pkey"
 AS OF SYSTEM TIME INTERVAL ''0 seconds''
 WHERE ((crdb_internal_expiration) <= ''2024-01-01 00:00:00'')
 AND (
-  (id >= 0)
+  (id >= 0::INT8)
 )
 AND (
-  (id <= 100)
+  (id <= 100::INT8)
 )
 ORDER BY id ASC
 LIMIT 500

pkg/sql/ttl/ttljob/ttljob.go

@@ -13,7 +13,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/jobs"
 	"github.com/cockroachdb/cockroach/pkg/jobs/joberror"
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
-	"github.com/cockroachdb/cockroach/pkg/kv"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/server/telemetry"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
@@ -59,8 +58,7 @@ func (t rowLevelTTLResumer) Resume(ctx context.Context, execCtx interface{}) (re
 
 	jobExecCtx := execCtx.(sql.JobExecContext)
 	execCfg := jobExecCtx.ExecCfg()
-	db := execCfg.DB
-	descsCol := jobExecCtx.ExtendedEvalContext().Descs
+	db := execCfg.InternalDB
 
 	settingsValues := execCfg.SV()
 	if err := ttlbase.CheckJobEnabled(settingsValues); err != nil {
@@ -84,8 +82,8 @@ func (t rowLevelTTLResumer) Resume(ctx context.Context, execCtx interface{}) (re
 	var rowLevelTTL *catpb.RowLevelTTL
 	var relationName string
 	var entirePKSpan roachpb.Span
-	if err := db.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
-		desc, err := descsCol.ByIDWithLeased(txn).WithoutNonPublic().Get().Table(ctx, details.TableID)
+	if err := db.DescsTxn(ctx, func(ctx context.Context, txn descs.Txn) error {
+		desc, err := txn.Descriptors().ByIDWithLeased(txn.KV()).WithoutNonPublic().Get().Table(ctx, details.TableID)
 		if err != nil {
 			return err
 		}
@@ -96,7 +94,7 @@ func (t rowLevelTTLResumer) Resume(ctx context.Context, execCtx interface{}) (re
 		if modificationTime.After(aost) {
 			return pgerror.Newf(
 				pgcode.ObjectNotInPrerequisiteState,
-				"found a recent schema change on the table at %s, aborting",
+				"found a recent schema change on the table at %s, job will run at the next scheduled time",
 				modificationTime.Format(time.RFC3339),
 			)
 		}
@@ -111,7 +109,7 @@ func (t rowLevelTTLResumer) Resume(ctx context.Context, execCtx interface{}) (re
 			return pgerror.Newf(pgcode.OperatorIntervention, "ttl jobs on table %s are currently paused", tree.Name(desc.GetName()))
 		}
 
-		tn, err := descs.GetObjectName(ctx, txn, descsCol, desc)
+		tn, err := descs.GetObjectName(ctx, txn.KV(), txn.Descriptors(), desc)
 		if err != nil {
 			return errors.Wrapf(err, "error fetching table relation name for TTL")
 		}