Merge cockroachdb#83134 cockroachdb#83615

83134: sql: remove `ttl_automatic_column` storage parameter r=otan a=ecwall fixes cockroachdb#83133 Release note (sql change): Remove ttl_automatic_column storage param. The crdb_internal_expiration column is created when ttl_expire_after is set and removed when ttl_expire_after is reset. 83615: sql: don't clear the memory account of the prepared stmt r=yuzefovich a=yuzefovich Previously, we had a bug of clearing the memory account of the prepared statements right after that prepared statement is created although we do keep the struct around. I believe this bug was introduced long time ago, and it could result in some memory not being accounted for. This is now fixed. Partially addresses: cockroachdb#72581. Release note: None Co-authored-by: Evan Wall <[email protected]> Co-authored-by: Yahor Yuzefovich <[email protected]>
ericharmeling · Jul 6, 2022 · 0848d90 · 0848d90
3 parents 41228d1 + 965f7c6 + ac40016
commit 0848d90
Show file tree

Hide file tree

Showing 13 changed files with 92 additions and 135 deletions.
diff --git a/pkg/ccl/backupccl/testdata/backup-restore/row_level_ttl b/pkg/ccl/backupccl/testdata/backup-restore/row_level_ttl
@@ -39,7 +39,7 @@ CREATE TABLE public.t (
 	id INT8 NOT 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 t_pkey PRIMARY KEY (id ASC)
-) WITH (ttl = 'on', ttl_automatic_column = 'on', ttl_expire_after = '00:10:00':::INTERVAL, ttl_job_cron = '@hourly')
+) WITH (ttl = 'on', ttl_expire_after = '00:10:00':::INTERVAL, ttl_job_cron = '@hourly')
 
 query-sql
 SELECT count(1) FROM [SHOW SCHEDULES]
@@ -72,7 +72,7 @@ CREATE TABLE public.t (
 	id INT8 NOT 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 t_pkey PRIMARY KEY (id ASC)
-) WITH (ttl = 'on', ttl_automatic_column = 'on', ttl_expire_after = '00:10:00':::INTERVAL, ttl_job_cron = '@hourly')
+) WITH (ttl = 'on', ttl_expire_after = '00:10:00':::INTERVAL, ttl_job_cron = '@hourly')
 
 query-sql
 SELECT count(1) FROM [SHOW SCHEDULES]
@@ -109,7 +109,7 @@ CREATE TABLE public.t (
 	id INT8 NOT 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 t_pkey PRIMARY KEY (id ASC)
-) WITH (ttl = 'on', ttl_automatic_column = 'on', ttl_expire_after = '00:10:00':::INTERVAL, ttl_job_cron = '@hourly')
+) WITH (ttl = 'on', ttl_expire_after = '00:10:00':::INTERVAL, ttl_job_cron = '@hourly')
 
 query-sql
 SELECT count(1) FROM [SHOW SCHEDULES]

diff --git a/pkg/sql/alter_table.go b/pkg/sql/alter_table.go
@@ -733,7 +733,6 @@ func (n *alterTableNode) startExec(params runParams) error {
 				ttlBefore = protoutil.Clone(ttl).(*catpb.RowLevelTTL)
 			}
 			if err := storageparam.Set(
-				params.ctx,
 				params.p.SemaCtx(),
 				params.EvalContext(),
 				t.StorageParams,
@@ -767,7 +766,6 @@ func (n *alterTableNode) startExec(params runParams) error {
 				ttlBefore = protoutil.Clone(ttl).(*catpb.RowLevelTTL)
 			}
 			if err := storageparam.Reset(
-				params.ctx,
 				params.EvalContext(),
 				t.Params,
 				tablestorageparam.NewSetter(n.tableDesc),

diff --git a/pkg/sql/catalog/tabledesc/structured.go b/pkg/sql/catalog/tabledesc/structured.go
@@ -2567,7 +2567,6 @@ func (desc *wrapper) GetStorageParams(spaceBetweenEqual bool) []string {
 	if ttl := desc.GetRowLevelTTL(); ttl != nil {
 		appendStorageParam(`ttl`, `'on'`)
 		if ttl.HasDurationExpr() {
-			appendStorageParam(`ttl_automatic_column`, `'on'`)
 			appendStorageParam(`ttl_expire_after`, string(ttl.DurationExpr))
 		}
 		if ttl.HasExpirationExpr() {

diff --git a/pkg/sql/conn_executor_prepare.go b/pkg/sql/conn_executor_prepare.go
@@ -153,9 +153,6 @@ func (ex *connExecutor) prepare(
 		createdAt: timeutil.Now(),
 		origin:    origin,
 	}
-	// NB: if we start caching the plan, we'll want to keep around the memory
-	// account used for the plan, rather than clearing it.
-	defer prepared.memAcc.Clear(ctx)
 
 	if stmt.AST == nil {
 		return prepared, nil

diff --git a/pkg/sql/create_index.go b/pkg/sql/create_index.go
@@ -280,7 +280,6 @@ func makeIndexDescriptor(
 	}
 
 	if err := storageparam.Set(
-		params.ctx,
 		params.p.SemaCtx(),
 		params.EvalContext(),
 		n.StorageParams,

diff --git a/pkg/sql/create_table.go b/pkg/sql/create_table.go
@@ -1325,7 +1325,6 @@ func NewTableDesc(
 	)
 
 	if err := storageparam.Set(
-		ctx,
 		semaCtx,
 		evalCtx,
 		n.StorageParams,
@@ -1863,7 +1862,6 @@ func NewTableDesc(
 				idx.Predicate = expr
 			}
 			if err := storageparam.Set(
-				ctx,
 				semaCtx,
 				evalCtx,
 				d.StorageParams,

diff --git a/pkg/sql/logictest/testdata/logic_test/row_level_ttl b/pkg/sql/logictest/testdata/logic_test/row_level_ttl
diff --git a/pkg/sql/schema_changer_test.go b/pkg/sql/schema_changer_test.go
@@ -7605,7 +7605,7 @@ CREATE TABLE t.test (id TEXT PRIMARY KEY) WITH (ttl_expire_after = '10 hours');`
 	id STRING NOT NULL,
 	crdb_internal_expiration TIMESTAMPTZ NOT VISIBLE NOT NULL DEFAULT current_timestamp():::TIMESTAMPTZ + '10:00:00':::INTERVAL ON UPDATE current_timestamp():::TIMESTAMPTZ + '10:00:00':::INTERVAL,
 	CONSTRAINT test_pkey PRIMARY KEY (id ASC)
-) WITH (ttl = 'on', ttl_automatic_column = 'on', ttl_expire_after = '10:00:00':::INTERVAL, ttl_job_cron = '@hourly')`
+) WITH (ttl = 'on', ttl_expire_after = '10:00:00':::INTERVAL, ttl_job_cron = '@hourly')`
 	)
 
 	testCases := []struct {

diff --git a/pkg/sql/sem/builtins/geo_builtins.go b/pkg/sql/sem/builtins/geo_builtins.go
@@ -7674,7 +7674,6 @@ func applyGeoindexConfigStorageParams(
 	}
 	semaCtx := tree.MakeSemaContext()
 	if err := storageparam.Set(
-		evalCtx.Context,
 		&semaCtx,
 		evalCtx,
 		stmt.AST.(*tree.CreateIndex).StorageParams,

diff --git a/pkg/sql/show_test.go b/pkg/sql/show_test.go
@@ -165,7 +165,7 @@ func TestShowCreateTable(t *testing.T) {
 	pk INT8 NOT 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 %[1]s_pkey PRIMARY KEY (pk ASC)
-) WITH (ttl = 'on', ttl_automatic_column = 'on', ttl_expire_after = '00:10:00':::INTERVAL, ttl_job_cron = '@hourly')`,
+) WITH (ttl = 'on', ttl_expire_after = '00:10:00':::INTERVAL, ttl_job_cron = '@hourly')`,
 		},
 		// Check that FK dependencies inside the current database
 		// have their db name omitted.

diff --git a/pkg/sql/storageparam/indexstorageparam/index_storage_param.go b/pkg/sql/storageparam/indexstorageparam/index_storage_param.go
@@ -13,8 +13,6 @@
 package indexstorageparam
 
 import (
-	"context"
-
 	"github.com/cockroachdb/cockroach/pkg/geo/geoindex"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/paramparse"
@@ -109,11 +107,7 @@ func (po *Setter) applyGeometryIndexSetting(
 
 // Set implements the Setter interface.
 func (po *Setter) Set(
-	ctx context.Context,
-	semaCtx *tree.SemaContext,
-	evalCtx *eval.Context,
-	key string,
-	expr tree.Datum,
+	semaCtx *tree.SemaContext, evalCtx *eval.Context, key string, expr tree.Datum,
 ) error {
 	switch key {
 	case `fillfactor`:

diff --git a/pkg/sql/storageparam/storage_param.go b/pkg/sql/storageparam/storage_param.go
@@ -13,8 +13,6 @@
 package storageparam
 
 import (
-	"context"
-
 	"github.com/cockroachdb/cockroach/pkg/server/telemetry"
 	"github.com/cockroachdb/cockroach/pkg/sql/paramparse"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
@@ -31,7 +29,7 @@ import (
 type Setter interface {
 	// Set is called during CREATE [TABLE | INDEX] ... WITH (...) or
 	// ALTER [TABLE | INDEX] ... WITH (...).
-	Set(ctx context.Context, semaCtx *tree.SemaContext, evalCtx *eval.Context, key string, datum tree.Datum) error
+	Set(semaCtx *tree.SemaContext, evalCtx *eval.Context, key string, datum tree.Datum) error
 	// Reset is called during ALTER [TABLE | INDEX] ... RESET (...)
 	Reset(evalCtx *eval.Context, key string) error
 	// RunPostChecks is called after all storage parameters have been set.
@@ -43,11 +41,7 @@ type Setter interface {
 // Set sets the given storage parameters using the
 // given observer.
 func Set(
-	ctx context.Context,
-	semaCtx *tree.SemaContext,
-	evalCtx *eval.Context,
-	params tree.StorageParams,
-	setter Setter,
+	semaCtx *tree.SemaContext, evalCtx *eval.Context, params tree.StorageParams, setter Setter,
 ) error {
 	for _, sp := range params {
 		key := string(sp.Key)
@@ -61,7 +55,7 @@ func Set(
 		expr := paramparse.UnresolvedNameToStrVal(sp.Value)
 
 		// Convert the expressions to a datum.
-		typedExpr, err := tree.TypeCheck(ctx, expr, semaCtx, types.Any)
+		typedExpr, err := tree.TypeCheck(evalCtx.Context, expr, semaCtx, types.Any)
 		if err != nil {
 			return err
 		}
@@ -73,7 +67,7 @@ func Set(
 			return err
 		}
 
-		if err := setter.Set(ctx, semaCtx, evalCtx, key, datum); err != nil {
+		if err := setter.Set(semaCtx, evalCtx, key, datum); err != nil {
 			return err
 		}
 	}
@@ -82,9 +76,7 @@ func Set(
 
 // Reset sets the given storage parameters using the
 // given observer.
-func Reset(
-	ctx context.Context, evalCtx *eval.Context, params tree.NameList, paramObserver Setter,
-) error {
+func Reset(evalCtx *eval.Context, params tree.NameList, paramObserver Setter) error {
 	for _, p := range params {
 		telemetry.Inc(sqltelemetry.ResetTableStorageParameter(string(p)))
 		if err := paramObserver.Reset(evalCtx, string(p)); err != nil {