From 6d9404220d4b379ef53a2decf02e892890447870 Mon Sep 17 00:00:00 2001 From: Jeff Date: Thu, 20 Jul 2023 21:45:40 +0000 Subject: [PATCH 1/4] sqlproxyccl: simplify NewSubStopper The lock in NewSubStopper caused lock ordering warnings when run under deadlock detection. The justification given for the lock's existence is wrong. If a closer is added to an already stopped stopper, the closer is called immediately. Release note: None Fixes: #106571 --- .../tenantdirsvr/test_directory_svr.go | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-) diff --git a/pkg/ccl/sqlproxyccl/tenantdirsvr/test_directory_svr.go b/pkg/ccl/sqlproxyccl/tenantdirsvr/test_directory_svr.go index 980c2d40418c..d64f2a1349cb 100644 --- a/pkg/ccl/sqlproxyccl/tenantdirsvr/test_directory_svr.go +++ b/pkg/ccl/sqlproxyccl/tenantdirsvr/test_directory_svr.go @@ -29,26 +29,12 @@ import ( var _ tenant.DirectoryServer = (*TestDirectoryServer)(nil) // NewSubStopper creates a new stopper that will be stopped when either the -// parent is stopped or its own Stop is called. The code is slightly more -// complicated that simply calling NewStopper followed by AddCloser since there -// is a possibility that between the two calls, the parent stopper completes a -// stop and then the leak detection may find a leaked stopper. +// parent is stopped or its own Stop is called. func NewSubStopper(parentStopper *stop.Stopper) *stop.Stopper { - var mu syncutil.Mutex - var subStopper *stop.Stopper + subStopper := stop.NewStopper(stop.WithTracer(parentStopper.Tracer())) parentStopper.AddCloser(stop.CloserFn(func() { - mu.Lock() - defer mu.Unlock() - if subStopper == nil { - subStopper = stop.NewStopper(stop.WithTracer(parentStopper.Tracer())) - } subStopper.Stop(context.Background()) })) - mu.Lock() - defer mu.Unlock() - if subStopper == nil { - subStopper = stop.NewStopper(stop.WithTracer(parentStopper.Tracer())) - } return subStopper } From fc5920459f4cc0950f195b9394c43fd07e8c9120 Mon Sep 17 00:00:00 2001 From: Chris Seto Date: Wed, 26 Jul 2023 16:59:40 -0400 Subject: [PATCH 2/4] ttl: show table name for jobs in SHOW SCHEDULES Previously, `SHOW SCHEDULES` would only show a table's ID for row level TTL jobs. For the convenience of debugging, this commit upgrades the `label` column to contain the table's name instead of its ID. Rather than performing table name resolution at query time, the job name is set at creation time and updated by the schema change during table renames. This minimizes the likelihood of `SHOW SCHEDULES` being rendered unusable when a cluster is in an unstable state. Epic: CRDB-18322 Fixes: 93774 --- pkg/ccl/backupccl/restore_job.go | 3 +- .../restore-on-fail-or-cancel-ttl | 4 +- .../testdata/backup-restore/row_level_ttl | 14 +- pkg/sql/BUILD.bazel | 1 + pkg/sql/check.go | 3 +- pkg/sql/create_table.go | 27 +-- pkg/sql/descmetadata/BUILD.bazel | 2 + pkg/sql/descmetadata/metadata_updater.go | 22 +++ .../testdata/logic_test/row_level_ttl | 169 +++++++++++++----- pkg/sql/rename_table.go | 16 ++ pkg/sql/schema_changer.go | 3 +- pkg/sql/schema_changer_test.go | 14 +- .../scdeps/sctestdeps/test_deps.go | 7 + pkg/sql/schemachanger/scexec/BUILD.bazel | 1 + pkg/sql/schemachanger/scexec/dependencies.go | 5 + .../scexec/executor_external_test.go | 9 +- pkg/sql/ttl/ttlbase/BUILD.bazel | 1 + pkg/sql/ttl/ttlbase/ttl_helpers.go | 8 + pkg/sql/ttl/ttlschedule/ttlschedule.go | 3 + 19 files changed, 232 insertions(+), 80 deletions(-) diff --git a/pkg/ccl/backupccl/restore_job.go b/pkg/ccl/backupccl/restore_job.go index 9bdef7d2384b..88c9a2c6b942 100644 --- a/pkg/ccl/backupccl/restore_job.go +++ b/pkg/ccl/backupccl/restore_job.go @@ -2239,8 +2239,7 @@ func (r *restoreResumer) publishDescriptors( jobsKnobs, jobs.ScheduledJobTxn(txn), user, - mutTable.GetID(), - mutTable.GetRowLevelTTL(), + mutTable, ) if err != nil { return err diff --git a/pkg/ccl/backupccl/testdata/backup-restore/restore-on-fail-or-cancel-ttl b/pkg/ccl/backupccl/testdata/backup-restore/restore-on-fail-or-cancel-ttl index 89b29fdb1d3a..0c1e9b897564 100644 --- a/pkg/ccl/backupccl/testdata/backup-restore/restore-on-fail-or-cancel-ttl +++ b/pkg/ccl/backupccl/testdata/backup-restore/restore-on-fail-or-cancel-ttl @@ -34,7 +34,7 @@ job cancel=a ---- query-sql -SELECT count(1) FROM [SHOW SCHEDULES] WHERE label LIKE 'row-level-ttl-%'; +SELECT count(1) FROM [SHOW SCHEDULES] WHERE label LIKE 'row-level-ttl%'; ---- 0 @@ -75,7 +75,7 @@ job cancel=b ---- query-sql -SELECT count(1) FROM [SHOW SCHEDULES] WHERE label LIKE 'row-level-ttl-%'; +SELECT count(1) FROM [SHOW SCHEDULES] WHERE label LIKE 'row-level-ttl%'; ---- 0 diff --git a/pkg/ccl/backupccl/testdata/backup-restore/row_level_ttl b/pkg/ccl/backupccl/testdata/backup-restore/row_level_ttl index ed3389646ecc..8409720584fb 100644 --- a/pkg/ccl/backupccl/testdata/backup-restore/row_level_ttl +++ b/pkg/ccl/backupccl/testdata/backup-restore/row_level_ttl @@ -20,7 +20,7 @@ new-cluster name=s2 share-io-dir=s1 allow-implicit-access query-sql SELECT count(1) FROM [SHOW SCHEDULES] -WHERE label LIKE 'row-level-ttl-%' +WHERE label LIKE 'row-level-ttl%' ---- 0 @@ -43,7 +43,7 @@ CREATE TABLE public.t ( query-sql SELECT count(1) FROM [SHOW SCHEDULES] -WHERE label LIKE 'row-level-ttl-%' +WHERE label LIKE 'row-level-ttl%' ---- 1 @@ -53,7 +53,7 @@ DROP DATABASE d query-sql SELECT count(1) FROM [SHOW SCHEDULES] -WHERE label LIKE 'row-level-ttl-%' +WHERE label LIKE 'row-level-ttl%' ---- 0 @@ -76,7 +76,7 @@ CREATE TABLE public.t ( query-sql SELECT count(1) FROM [SHOW SCHEDULES] -WHERE label LIKE 'row-level-ttl-%' +WHERE label LIKE 'row-level-ttl%' ---- 1 @@ -86,7 +86,7 @@ DROP DATABASE d query-sql SELECT count(1) FROM [SHOW SCHEDULES] -WHERE label LIKE 'row-level-ttl-%' +WHERE label LIKE 'row-level-ttl%' ---- 0 @@ -113,7 +113,7 @@ CREATE TABLE public.t ( query-sql SELECT count(1) FROM [SHOW SCHEDULES] -WHERE label LIKE 'row-level-ttl-%' +WHERE label LIKE 'row-level-ttl%' ---- 1 @@ -123,6 +123,6 @@ DROP TABLE d.public.t query-sql SELECT count(1) FROM [SHOW SCHEDULES] -WHERE label LIKE 'row-level-ttl-%' +WHERE label LIKE 'row-level-ttl%' ---- 0 diff --git a/pkg/sql/BUILD.bazel b/pkg/sql/BUILD.bazel index 97b8876432e4..d6ad4bdfbb30 100644 --- a/pkg/sql/BUILD.bazel +++ b/pkg/sql/BUILD.bazel @@ -508,6 +508,7 @@ go_library( "//pkg/sql/storageparam/tablestorageparam", "//pkg/sql/syntheticprivilege", "//pkg/sql/syntheticprivilegecache", + "//pkg/sql/ttl/ttlbase", "//pkg/sql/types", "//pkg/sql/vtable", "//pkg/storage", diff --git a/pkg/sql/check.go b/pkg/sql/check.go index 8e1d4f91122e..6e24b271b858 100644 --- a/pkg/sql/check.go +++ b/pkg/sql/check.go @@ -799,8 +799,7 @@ func (p *planner) RepairTTLScheduledJobForTable(ctx context.Context, tableID int p.ExecCfg().JobsKnobs(), jobs.ScheduledJobTxn(p.InternalSQLTxn()), p.User(), - tableDesc.GetID(), - tableDesc.GetRowLevelTTL(), + tableDesc, ) if err != nil { return err diff --git a/pkg/sql/create_table.go b/pkg/sql/create_table.go index 80c929e1798a..7b3ecf9bad2a 100644 --- a/pkg/sql/create_table.go +++ b/pkg/sql/create_table.go @@ -59,6 +59,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/storageparam" "github.com/cockroachdb/cockroach/pkg/sql/storageparam/indexstorageparam" "github.com/cockroachdb/cockroach/pkg/sql/storageparam/tablestorageparam" + "github.com/cockroachdb/cockroach/pkg/sql/ttl/ttlbase" "github.com/cockroachdb/cockroach/pkg/sql/types" "github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented" "github.com/cockroachdb/cockroach/pkg/util/hlc" @@ -2416,8 +2417,7 @@ func newTableDesc( params.ExecCfg().JobsKnobs(), jobs.ScheduledJobTxn(params.p.InternalSQLTxn()), params.p.User(), - ret.GetID(), - ttl, + ret, ) if err != nil { return nil, err @@ -2428,15 +2428,13 @@ func newTableDesc( } // newRowLevelTTLScheduledJob returns a *jobs.ScheduledJob for row level TTL -// for a given table. +// for a given table. newRowLevelTTLScheduledJob assumes that +// tblDesc.RowLevelTTL is not nil. func newRowLevelTTLScheduledJob( - env scheduledjobs.JobSchedulerEnv, - owner username.SQLUsername, - tblID descpb.ID, - ttl *catpb.RowLevelTTL, + env scheduledjobs.JobSchedulerEnv, owner username.SQLUsername, tblDesc *tabledesc.Mutable, ) (*jobs.ScheduledJob, error) { sj := jobs.NewScheduledJob(env) - sj.SetScheduleLabel(fmt.Sprintf("row-level-ttl-%d", tblID)) + sj.SetScheduleLabel(ttlbase.BuildScheduleLabel(tblDesc)) sj.SetOwner(owner) sj.SetScheduleDetails(jobspb.ScheduleDetails{ Wait: jobspb.ScheduleDetails_WAIT, @@ -2444,11 +2442,11 @@ func newRowLevelTTLScheduledJob( OnError: jobspb.ScheduleDetails_RETRY_SCHED, }) - if err := sj.SetSchedule(ttl.DeletionCronOrDefault()); err != nil { + if err := sj.SetSchedule(tblDesc.RowLevelTTL.DeletionCronOrDefault()); err != nil { return nil, err } args := &catpb.ScheduledRowLevelTTLArgs{ - TableID: tblID, + TableID: tblDesc.GetID(), } any, err := pbtypes.MarshalAny(args) if err != nil { @@ -2467,12 +2465,15 @@ func CreateRowLevelTTLScheduledJob( knobs *jobs.TestingKnobs, s jobs.ScheduledJobStorage, owner username.SQLUsername, - tblID descpb.ID, - ttl *catpb.RowLevelTTL, + tblDesc *tabledesc.Mutable, ) (*jobs.ScheduledJob, error) { + if !tblDesc.HasRowLevelTTL() { + return nil, errors.AssertionFailedf("CreateRowLevelTTLScheduledJob called with no .RowLevelTTL: %#v", tblDesc) + } + telemetry.Inc(sqltelemetry.RowLevelTTLCreated) env := JobSchedulerEnv(knobs) - j, err := newRowLevelTTLScheduledJob(env, owner, tblID, ttl) + j, err := newRowLevelTTLScheduledJob(env, owner, tblDesc) if err != nil { return nil, err } diff --git a/pkg/sql/descmetadata/BUILD.bazel b/pkg/sql/descmetadata/BUILD.bazel index 09368a5f89b0..58cacf05393c 100644 --- a/pkg/sql/descmetadata/BUILD.bazel +++ b/pkg/sql/descmetadata/BUILD.bazel @@ -10,10 +10,12 @@ go_library( "//pkg/settings", "//pkg/sql/catalog/descpb", "//pkg/sql/catalog/descs", + "//pkg/sql/catalog/tabledesc", "//pkg/sql/isql", "//pkg/sql/schemachanger/scexec", "//pkg/sql/sessiondata", "//pkg/sql/sessioninit", + "//pkg/sql/ttl/ttlbase", ], ) diff --git a/pkg/sql/descmetadata/metadata_updater.go b/pkg/sql/descmetadata/metadata_updater.go index f9767e8a25b4..f764571180ac 100644 --- a/pkg/sql/descmetadata/metadata_updater.go +++ b/pkg/sql/descmetadata/metadata_updater.go @@ -18,10 +18,12 @@ import ( "github.com/cockroachdb/cockroach/pkg/settings" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" "github.com/cockroachdb/cockroach/pkg/sql/isql" "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scexec" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/cockroach/pkg/sql/sessioninit" + "github.com/cockroachdb/cockroach/pkg/sql/ttl/ttlbase" ) // metadataUpdater which implements scexec.MetaDataUpdater that is used to update @@ -94,3 +96,23 @@ func (mu metadataUpdater) DeleteSchedule(ctx context.Context, scheduleID int64) ) return err } + +// UpdateTTLScheduleLabel implement scexec.DescriptorMetadataUpdater. +func (mu metadataUpdater) UpdateTTLScheduleLabel( + ctx context.Context, tbl *tabledesc.Mutable, +) error { + if !tbl.HasRowLevelTTL() { + return nil + } + + _, err := mu.txn.ExecEx( + ctx, + "update-ttl-schedule-label", + mu.txn.KV(), + sessiondata.RootUserSessionDataOverride, + "UPDATE system.scheduled_jobs SET schedule_name = $1 WHERE schedule_id = $2", + ttlbase.BuildScheduleLabel(tbl), + tbl.RowLevelTTL.ScheduleID, + ) + return err +} diff --git a/pkg/sql/logictest/testdata/logic_test/row_level_ttl b/pkg/sql/logictest/testdata/logic_test/row_level_ttl index a51071436acd..8c390e72ef37 100644 --- a/pkg/sql/logictest/testdata/logic_test/row_level_ttl +++ b/pkg/sql/logictest/testdata/logic_test/row_level_ttl @@ -233,17 +233,17 @@ CREATE TABLE tbl_schedules ( id INT PRIMARY KEY ) WITH (ttl_expire_after = '10 minutes') -let $table_id -SELECT oid FROM pg_class WHERE relname = 'tbl_schedules' +let $label_suffix +SELECT relname || ' [' || oid || ']' FROM pg_class WHERE relname = 'tbl_schedules' query I SELECT count(1) FROM [SHOW SCHEDULES] -WHERE label = 'row-level-ttl-$table_id' +WHERE label = 'row-level-ttl: $label_suffix' ---- 1 let $schedule_id -SELECT id FROM [SHOW SCHEDULES] WHERE label = 'row-level-ttl-$table_id' +SELECT id FROM [SHOW SCHEDULES] WHERE label = 'row-level-ttl: $label_suffix' statement error cannot drop a row level TTL schedule\nHINT: use ALTER TABLE test\.public\.tbl_schedules RESET \(ttl\) instead DROP SCHEDULE $schedule_id @@ -321,12 +321,12 @@ CREATE TABLE tbl_reset_ttl ( id INT PRIMARY KEY ) WITH (ttl_expire_after = '10 minutes') -let $table_id -SELECT oid FROM pg_class WHERE relname = 'tbl_reset_ttl' +let $label_suffix +SELECT relname || ' [' || oid || ']' FROM pg_class WHERE relname = 'tbl_reset_ttl' query I SELECT count(1) FROM [SHOW SCHEDULES] -WHERE label = 'row-level-ttl-$table_id' +WHERE label = 'row-level-ttl: $label_suffix' ---- 1 @@ -351,12 +351,92 @@ SELECT crdb_internal.validate_ttl_scheduled_jobs() query I SELECT count(1) FROM [SHOW SCHEDULES] -WHERE label = 'row-level-ttl-$table_id' +WHERE label = 'row-level-ttl: $label_suffix' ---- 0 subtest end +# Ensure that SHOW SCHEDULES's label column is updated to reflect the new table +# name. +subtest rename_table + +statement ok +CREATE TABLE tbl_rename ( + id INT PRIMARY KEY +) WITH (ttl_expire_after = '10 minutes') + +let $label_suffix +SELECT relname || ' [' || oid || ']' FROM pg_class WHERE relname = 'tbl_rename' + +query I +SELECT count(1) FROM [SHOW SCHEDULES] +WHERE label = 'row-level-ttl: $label_suffix' +---- +1 + +statement ok +ALTER TABLE tbl_rename RENAME TO tbl_renamed + +query I +SELECT count(1) FROM [SHOW SCHEDULES] +WHERE label = 'row-level-ttl: $label_suffix' +---- +0 + +let $label_suffix +SELECT relname || ' [' || oid || ']' FROM pg_class WHERE relname = 'tbl_renamed' + +query I +SELECT count(1) FROM [SHOW SCHEDULES] +WHERE label = 'row-level-ttl: $label_suffix' +---- +1 + +subtest end + +subtest rename_table_legacy_label + +statement ok +CREATE TABLE tbl_rename_legacy_label ( + id INT PRIMARY KEY +) WITH (ttl_expire_after = '10 minutes') + +let $label_suffix +SELECT relname || ' [' || oid || ']' FROM pg_class WHERE relname = 'tbl_rename_legacy_label' + +query I +SELECT count(1) FROM [SHOW SCHEDULES] +WHERE label = 'row-level-ttl: $label_suffix' +---- +1 + +# Simulate a "legacy" label to ensure that it will be appropriately updated +# when the table is renamed. +statement ok +UPDATE system.scheduled_jobs SET schedule_name = 'row-level-ttl-1234' WHERE schedule_name = 'row-level-ttl: $label_suffix'; + +statement ok +ALTER TABLE tbl_rename_legacy_label RENAME TO tbl_renamed2 + +let $label_suffix +SELECT relname || ' [' || oid || ']' FROM pg_class WHERE relname = 'tbl_renamed2' + +# Legacy label no longer exists. +query I +SELECT count(1) FROM [SHOW SCHEDULES] +WHERE label = 'row-level-ttl-1234' +---- +0 + +# Renamed and new formatted label is showing up as expected. +query I +SELECT count(1) FROM [SHOW SCHEDULES] +WHERE label = 'row-level-ttl: $label_suffix' +---- +1 +subtest end + subtest drop_table # Ensure schedules are removed on DROP TABLE. @@ -365,12 +445,13 @@ CREATE TABLE tbl_drop_table ( id INT PRIMARY KEY ) WITH (ttl_expire_after = '10 minutes') -let $table_id -SELECT oid FROM pg_class WHERE relname = 'tbl_drop_table' + +let $label_suffix +SELECT relname || ' [' || oid || ']' FROM pg_class WHERE relname = 'tbl_drop_table' query I SELECT count(1) FROM [SHOW SCHEDULES] -WHERE label = 'row-level-ttl-$table_id' +WHERE label = 'row-level-ttl: $label_suffix' ---- 1 @@ -379,7 +460,7 @@ DROP TABLE tbl_drop_table query I SELECT count(1) FROM [SHOW SCHEDULES] -WHERE label = 'row-level-ttl-$table_id' +WHERE label = 'row-level-ttl: $label_suffix' ---- 0 @@ -395,15 +476,15 @@ statement ok CREATE TABLE drop_me.tbl () WITH (ttl_expire_after = '10 minutes'); CREATE TABLE drop_me.tbl2 () WITH (ttl_expire_after = '10 minutes') -let $table_id -SELECT oid FROM pg_class WHERE relname = 'tbl' +let $label_suffix +SELECT relname || ' [' || oid || ']' FROM pg_class WHERE relname = 'tbl' -let $table_id2 -SELECT oid FROM pg_class WHERE relname = 'tbl2' +let $label_suffix2 +SELECT relname || ' [' || oid || ']' FROM pg_class WHERE relname = 'tbl2' query I SELECT count(1) FROM [SHOW SCHEDULES] -WHERE label IN ('row-level-ttl-$table_id', 'row-level-ttl-$table_id2') +WHERE label IN ('row-level-ttl: $label_suffix', 'row-level-ttl: $label_suffix2') ---- 2 @@ -412,7 +493,7 @@ DROP SCHEMA drop_me CASCADE query I SELECT count(1) FROM [SHOW SCHEDULES] -WHERE label = 'row-level-ttl-$table_id' +WHERE label = 'row-level-ttl: $label_suffix' ---- 0 @@ -431,15 +512,15 @@ statement ok CREATE TABLE tbl () WITH (ttl_expire_after = '10 minutes'); CREATE TABLE tbl2 () WITH (ttl_expire_after = '10 minutes') -let $table_id -SELECT oid FROM pg_class WHERE relname = 'tbl' +let $label_suffix +SELECT relname || ' [' || oid || ']' FROM pg_class WHERE relname = 'tbl' -let $table_id2 -SELECT oid FROM pg_class WHERE relname = 'tbl2' +let $label_suffix2 +SELECT relname || ' [' || oid || ']' FROM pg_class WHERE relname = 'tbl2' query I SELECT count(1) FROM [SHOW SCHEDULES] -WHERE label IN ('row-level-ttl-$table_id', 'row-level-ttl-$table_id2') +WHERE label IN ('row-level-ttl: $label_suffix', 'row-level-ttl: $label_suffix2') ---- 2 @@ -449,7 +530,7 @@ DROP DATABASE drop_me CASCADE query I SELECT count(1) FROM [SHOW SCHEDULES] -WHERE label = 'row-level-ttl-$table_id' +WHERE label = 'row-level-ttl: $label_suffix' ---- 0 @@ -494,18 +575,18 @@ CREATE TABLE public.tbl_ttl_on_noop ( CONSTRAINT tbl_ttl_on_noop_pkey PRIMARY KEY (id ASC) ) WITH (ttl = 'on', ttl_expire_after = '00:10:00':::INTERVAL, ttl_job_cron = '@hourly') -let $table_id -SELECT oid FROM pg_class WHERE relname = 'tbl_ttl_on_noop' +let $label_suffix +SELECT relname || ' [' || oid || ']' FROM pg_class WHERE relname = 'tbl_ttl_on_noop' query TTT SELECT schedule_status, recurrence, owner FROM [SHOW SCHEDULES] -WHERE label = 'row-level-ttl-$table_id' +WHERE label = 'row-level-ttl: $label_suffix' ---- ACTIVE @hourly root let $schedule_id SELECT id FROM [SHOW SCHEDULES] -WHERE label = 'row-level-ttl-$table_id' +WHERE label = 'row-level-ttl: $label_suffix' query T SELECT create_statement FROM [SHOW CREATE SCHEDULE $schedule_id] @@ -533,12 +614,12 @@ CREATE TABLE public.tbl_ttl_job_cron ( CONSTRAINT tbl_ttl_job_cron_pkey PRIMARY KEY (id ASC) ) WITH (ttl = 'on', ttl_expire_after = '00:10:00':::INTERVAL, ttl_job_cron = '@daily') -let $table_id -SELECT oid FROM pg_class WHERE relname = 'tbl_ttl_job_cron' +let $label_suffix +SELECT relname || ' [' || oid || ']' FROM pg_class WHERE relname = 'tbl_ttl_job_cron' query TTT SELECT schedule_status, recurrence, owner FROM [SHOW SCHEDULES] -WHERE label = 'row-level-ttl-$table_id' +WHERE label = 'row-level-ttl: $label_suffix' ---- ACTIVE @daily root @@ -557,7 +638,7 @@ CREATE TABLE public.tbl_ttl_job_cron ( query TTT SELECT schedule_status, recurrence, owner FROM [SHOW SCHEDULES] -WHERE label = 'row-level-ttl-$table_id' +WHERE label = 'row-level-ttl: $label_suffix' ---- ACTIVE @weekly root @@ -575,7 +656,7 @@ CREATE TABLE public.tbl_ttl_job_cron ( query TTT SELECT schedule_status, recurrence, owner FROM [SHOW SCHEDULES] -WHERE label = 'row-level-ttl-$table_id' +WHERE label = 'row-level-ttl: $label_suffix' ---- ACTIVE @hourly root @@ -1037,12 +1118,12 @@ CREATE TABLE public.create_table_no_ttl_set_ttl_expire_after ( CONSTRAINT create_table_no_ttl_set_ttl_expire_after_pkey PRIMARY KEY (id ASC) ) WITH (ttl = 'on', ttl_expire_after = '00:10:00':::INTERVAL, ttl_job_cron = '@hourly') -let $table_id -SELECT oid FROM pg_class WHERE relname = 'create_table_no_ttl_set_ttl_expire_after' +let $label_suffix +SELECT relname || ' [' || oid || ']' FROM pg_class WHERE relname = 'create_table_no_ttl_set_ttl_expire_after' query TTT SELECT schedule_status, recurrence, owner FROM [SHOW SCHEDULES] -WHERE label = 'row-level-ttl-$table_id' +WHERE label = 'row-level-ttl: $label_suffix' ---- ACTIVE @hourly root @@ -1051,7 +1132,7 @@ ALTER TABLE create_table_no_ttl_set_ttl_expire_after RESET (ttl) query TTT SELECT schedule_status, recurrence, owner FROM [SHOW SCHEDULES] -WHERE label = 'row-level-ttl-$table_id' +WHERE label = 'row-level-ttl: $label_suffix' ---- subtest end @@ -1078,12 +1159,12 @@ CREATE TABLE public.create_table_no_ttl_set_ttl_expiration_expression ( FAMILY fam_0_id_expire_at (id, expire_at) ) WITH (ttl = 'on', ttl_expiration_expression = 'expire_at', ttl_job_cron = '@hourly') -let $table_id -SELECT oid FROM pg_class WHERE relname = 'create_table_no_ttl_set_ttl_expiration_expression' +let $label_suffix +SELECT relname || ' [' || oid || ']' FROM pg_class WHERE relname = 'create_table_no_ttl_set_ttl_expiration_expression' query TTT SELECT schedule_status, recurrence, owner FROM [SHOW SCHEDULES] -WHERE label = 'row-level-ttl-$table_id' +WHERE label = 'row-level-ttl: $label_suffix' ---- ACTIVE @hourly root @@ -1092,7 +1173,7 @@ ALTER TABLE create_table_no_ttl_set_ttl_expiration_expression RESET (ttl) query TTT SELECT schedule_status, recurrence, owner FROM [SHOW SCHEDULES] -WHERE label = 'row-level-ttl-$table_id' +WHERE label = 'row-level-ttl: $label_suffix' ---- subtest end @@ -1102,12 +1183,12 @@ subtest special_table_name statement ok CREATE TABLE "Table-Name" (id INT PRIMARY KEY) WITH (ttl_expire_after = '10 hours') -let $table_id -SELECT oid FROM pg_class WHERE relname = 'Table-Name' +let $label_suffix +SELECT relname || ' [' || oid || ']' FROM pg_class WHERE relname = 'Table-Name' let $schedule_id SELECT id FROM [SHOW SCHEDULES] -WHERE label = 'row-level-ttl-$table_id' +WHERE label = 'row-level-ttl: $label_suffix' query T SELECT create_statement FROM [SHOW CREATE SCHEDULE $schedule_id] diff --git a/pkg/sql/rename_table.go b/pkg/sql/rename_table.go index 9262d930bf00..4d5e64a11610 100644 --- a/pkg/sql/rename_table.go +++ b/pkg/sql/rename_table.go @@ -17,6 +17,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" + "github.com/cockroachdb/cockroach/pkg/sql/descmetadata" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgnotice" @@ -232,6 +233,21 @@ func (n *renameTableNode) startExec(params runParams) error { return err } + metadataUpdater := descmetadata.NewMetadataUpdater( + ctx, + p.InternalSQLTxn(), + p.Descriptors(), + &p.ExecCfg().Settings.SV, + p.SessionData(), + ) + + // If this table has row level ttl enabled, update the schedule_name of all + // row-level-ttl jobs with the name table name. If row level TTL is not + // enabled, UpdateTTLScheduleName will no-op for us. + if err := metadataUpdater.UpdateTTLScheduleLabel(ctx, tableDesc); err != nil { + return err + } + // Log Rename Table event. This is an auditable log event and is recorded // in the same transaction as the table descriptor update. return p.logEvent(ctx, diff --git a/pkg/sql/schema_changer.go b/pkg/sql/schema_changer.go index d4fa2e03bce6..b8da88265894 100644 --- a/pkg/sql/schema_changer.go +++ b/pkg/sql/schema_changer.go @@ -1568,8 +1568,7 @@ func (sc *SchemaChanger) done(ctx context.Context) error { sc.execCfg.JobsKnobs(), scheduledJobs, scTable.GetPrivileges().Owner(), - scTable.GetID(), - modify.RowLevelTTL(), + scTable, ) if err != nil { return err diff --git a/pkg/sql/schema_changer_test.go b/pkg/sql/schema_changer_test.go index c9bb0fe548d8..800ffd491ddd 100644 --- a/pkg/sql/schema_changer_test.go +++ b/pkg/sql/schema_changer_test.go @@ -7802,26 +7802,26 @@ ALTER TABLE t.test SET (ttl_expire_after = '10 hours'); "t", "test", ) + rowLevelTTL := desc.GetRowLevelTTL() if tc.expectSchedule { require.NotNil(t, rowLevelTTL) require.Greater(t, rowLevelTTL.ScheduleID, int64(0)) // Ensure there is only one schedule and that it belongs to the table. + var hasSchedule bool + require.NoError(t, sqlDB.QueryRow(`SELECT count(1) = 1 FROM [SHOW SCHEDULES] WHERE id = $1`, rowLevelTTL.ScheduleID).Scan(&hasSchedule)) + require.True(t, hasSchedule) + var numSchedules int - require.NoError(t, sqlDB.QueryRow( - `SELECT count(1) FROM [SHOW SCHEDULES] WHERE label LIKE $1`, - fmt.Sprintf("row-level-ttl-%d", rowLevelTTL.ScheduleID), - ).Scan(&numSchedules)) - require.Equal(t, 0, numSchedules) - require.NoError(t, sqlDB.QueryRow(`SELECT count(1) FROM [SHOW SCHEDULES] WHERE label LIKE 'row-level-ttl-%'`).Scan(&numSchedules)) + require.NoError(t, sqlDB.QueryRow(`SELECT count(1) FROM [SHOW SCHEDULES] WHERE label LIKE SOME ('row-level-ttl%', '%' || $1 || '%', '%' || $2 || '%')`, desc.TableDesc().ID, desc.TableDesc().Name).Scan(&numSchedules)) require.Equal(t, 1, numSchedules) } else { require.Nil(t, rowLevelTTL) // Ensure there are no schedules. var numSchedules int - require.NoError(t, sqlDB.QueryRow(`SELECT count(1) FROM [SHOW SCHEDULES] WHERE label LIKE 'row-level-ttl-%'`).Scan(&numSchedules)) + require.NoError(t, sqlDB.QueryRow(`SELECT count(1) FROM [SHOW SCHEDULES] WHERE label LIKE ANY ('row-level-ttl%', '%' || $1 || '%', '%' || $2 || '%')`, desc.TableDesc().ID, desc.TableDesc().Name).Scan(&numSchedules)) require.Equal(t, 0, numSchedules) } }) diff --git a/pkg/sql/schemachanger/scdeps/sctestdeps/test_deps.go b/pkg/sql/schemachanger/scdeps/sctestdeps/test_deps.go index 79546a57c3b6..138d67c198c6 100644 --- a/pkg/sql/schemachanger/scdeps/sctestdeps/test_deps.go +++ b/pkg/sql/schemachanger/scdeps/sctestdeps/test_deps.go @@ -31,6 +31,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/funcdesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/nstree" "github.com/cockroachdb/cockroach/pkg/sql/catalog/schemadesc" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc" "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scbuild" @@ -1184,6 +1185,12 @@ func (s *TestState) DeleteSchedule(ctx context.Context, id int64) error { return nil } +// UpdateTTLScheduleLabel implements scexec.DescriptorMetadataUpdater +func (s *TestState) UpdateTTLScheduleLabel(ctx context.Context, tbl *tabledesc.Mutable) error { + s.LogSideEffectf("update ttl schedule label #%d", tbl.ID) + return nil +} + // DescriptorMetadataUpdater implement scexec.Dependencies. func (s *TestState) DescriptorMetadataUpdater( ctx context.Context, diff --git a/pkg/sql/schemachanger/scexec/BUILD.bazel b/pkg/sql/schemachanger/scexec/BUILD.bazel index 58e9f2cc4d72..b8338714284a 100644 --- a/pkg/sql/schemachanger/scexec/BUILD.bazel +++ b/pkg/sql/schemachanger/scexec/BUILD.bazel @@ -28,6 +28,7 @@ go_library( "//pkg/sql/catalog/catalogkeys", "//pkg/sql/catalog/descpb", "//pkg/sql/catalog/nstree", + "//pkg/sql/catalog/tabledesc", "//pkg/sql/pgwire/pgcode", "//pkg/sql/pgwire/pgerror", "//pkg/sql/schemachanger/scerrors", diff --git a/pkg/sql/schemachanger/scexec/dependencies.go b/pkg/sql/schemachanger/scexec/dependencies.go index 5480e95f08a0..8f8612470684 100644 --- a/pkg/sql/schemachanger/scexec/dependencies.go +++ b/pkg/sql/schemachanger/scexec/dependencies.go @@ -23,6 +23,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scexec/scmutationexec" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/cockroach/pkg/util/hlc" @@ -348,6 +349,10 @@ type DescriptorMetadataUpdater interface { // DeleteSchedule deletes the given schedule. DeleteSchedule(ctx context.Context, id int64) error + + // UpdateTTLScheduleLabel updates the schedule_name for the TTL Scheduled Job + // of the given table. + UpdateTTLScheduleLabel(ctx context.Context, tbl *tabledesc.Mutable) error } // StatsRefreshQueue queues table for stats refreshes. diff --git a/pkg/sql/schemachanger/scexec/executor_external_test.go b/pkg/sql/schemachanger/scexec/executor_external_test.go index 184c088aa303..8e6dd579f4ee 100644 --- a/pkg/sql/schemachanger/scexec/executor_external_test.go +++ b/pkg/sql/schemachanger/scexec/executor_external_test.go @@ -518,7 +518,14 @@ func (noopMetadataUpdater) DeleteDatabaseRoleSettings(ctx context.Context, dbID return nil } -// DeleteScheduleID implements scexec.DescriptorMetadataUpdater +// DeleteScheduleID implements scexec.DescriptorMetadataUpdater. func (noopMetadataUpdater) DeleteSchedule(ctx context.Context, scheduleID int64) error { return nil } + +// UpdateTTLScheduleLabel implements scexec.DescriptorMetadataUpdater. +func (noopMetadataUpdater) UpdateTTLScheduleLabel( + ctx context.Context, tbl *tabledesc.Mutable, +) error { + return nil +} diff --git a/pkg/sql/ttl/ttlbase/BUILD.bazel b/pkg/sql/ttl/ttlbase/BUILD.bazel index 77f9ed939df6..e78de32d4222 100644 --- a/pkg/sql/ttl/ttlbase/BUILD.bazel +++ b/pkg/sql/ttl/ttlbase/BUILD.bazel @@ -11,6 +11,7 @@ go_library( deps = [ "//pkg/sql/catalog/catenumpb", "//pkg/sql/catalog/catpb", + "//pkg/sql/catalog/tabledesc", ], ) diff --git a/pkg/sql/ttl/ttlbase/ttl_helpers.go b/pkg/sql/ttl/ttlbase/ttl_helpers.go index c39e2f6cb266..6de77ee37ded 100644 --- a/pkg/sql/ttl/ttlbase/ttl_helpers.go +++ b/pkg/sql/ttl/ttlbase/ttl_helpers.go @@ -12,11 +12,13 @@ package ttlbase import ( "bytes" + "fmt" "strconv" "time" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catenumpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" ) // DefaultAOSTDuration is the default duration to use in the AS OF SYSTEM TIME @@ -32,6 +34,12 @@ var endKeyCompareOps = map[catenumpb.IndexColumn_Direction]string{ catenumpb.IndexColumn_DESC: ">", } +// BuildScheduleLabel returns a string value intended for use as the +// schedule_name/label column for the scheduled job created by row level TTL. +func BuildScheduleLabel(tbl *tabledesc.Mutable) string { + return fmt.Sprintf("row-level-ttl: %s [%d]", tbl.GetName(), tbl.ID) +} + func BuildSelectQuery( relationName string, pkColNames []string, diff --git a/pkg/sql/ttl/ttlschedule/ttlschedule.go b/pkg/sql/ttl/ttlschedule/ttlschedule.go index 08f0b576710f..64ec0502da98 100644 --- a/pkg/sql/ttl/ttlschedule/ttlschedule.go +++ b/pkg/sql/ttl/ttlschedule/ttlschedule.go @@ -136,6 +136,9 @@ func (s rowLevelTTLExecutor) ExecuteJob( return err } + // TODO(chrisseto): Should opName be updated to match schedule_name? We'll + // have to query to resolve the table name or delegate to sj.ScheduleLabel, + // which may make debugging quite confusing if the label gets out of whack. p, cleanup := cfg.PlanHookMaker( ctx, fmt.Sprintf("invoke-row-level-ttl-%d", args.TableID), From b67f136c757d38114dcd888ca6b0f363239b7f6a Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Tue, 1 Aug 2023 12:49:48 +0000 Subject: [PATCH 3/4] storage: don't import `testing` for `DisableMetamorphicSimpleValueEncoding` We shouldn't link `testing` in binaries. Epic: none Release note: None --- pkg/storage/mvcc_value.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/storage/mvcc_value.go b/pkg/storage/mvcc_value.go index d671a7dec8ec..0718418b8a12 100644 --- a/pkg/storage/mvcc_value.go +++ b/pkg/storage/mvcc_value.go @@ -12,7 +12,6 @@ package storage import ( "encoding/binary" - "testing" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/storage/enginepb" @@ -147,7 +146,10 @@ var disableSimpleValueEncoding = util.ConstantWithMetamorphicTestBool( // DisableMetamorphicSimpleValueEncoding disables the disableSimpleValueEncoding // metamorphic bool for the duration of a test, resetting it at the end. -func DisableMetamorphicSimpleValueEncoding(t testing.TB) { +func DisableMetamorphicSimpleValueEncoding(t interface { + Helper() + Cleanup(func()) +}) { t.Helper() if disableSimpleValueEncoding { disableSimpleValueEncoding = false From 1395ba862e5a350bf6f163ad7545b34a769caeef Mon Sep 17 00:00:00 2001 From: Renato Costa Date: Tue, 1 Aug 2023 11:06:33 -0400 Subject: [PATCH 4/4] testutils: fix determinism in random predecessor history tests Previously, the `rng` instance used by the tests was initialized once in the `var` block. That meant that every assertion that relied on that rng was order-dependent: running a single test in that file in isolation would lead to failed assertions. This commit ensures that every test resets the rng before making any assertions, guaranteeing we get the same values regardless of execution order. Epic: none Release note: None --- pkg/testutils/release/releases_test.go | 35 ++++++++++++++++---------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/pkg/testutils/release/releases_test.go b/pkg/testutils/release/releases_test.go index 8bfbb91ff00c..f5073d3182c0 100644 --- a/pkg/testutils/release/releases_test.go +++ b/pkg/testutils/release/releases_test.go @@ -40,10 +40,17 @@ var ( }, } - // Results rely on this constant seed - rng = rand.New(rand.NewSource(1)) + // Results rely on this constant seed. + seed = int64(1) + + // Tests must call `resetRNG` if they use this global rng. + rng *rand.Rand ) +func resetRNG() { + rng = rand.New(rand.NewSource(seed)) +} + func TestLatestAndRandomPredecessor(t *testing.T) { testCases := []struct { name string @@ -72,16 +79,17 @@ func TestLatestAndRandomPredecessor(t *testing.T) { name: "latest is withdrawn", v: "v22.2.3", expectedLatest: "22.1.12", - expectedRandom: "22.1.8", + expectedRandom: "22.1.10", }, } - for _, tc := range testCases { - oldReleaseData := releaseData - releaseData = testReleaseData - defer func() { releaseData = oldReleaseData }() + oldReleaseData := releaseData + releaseData = testReleaseData + defer func() { releaseData = oldReleaseData }() + for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { + resetRNG() // deterministic results latestPred, latestErr := LatestPredecessor(version.MustParse(tc.v)) randomPred, randomErr := RandomPredecessor(rng, version.MustParse(tc.v)) if tc.expectedErr == "" { @@ -119,23 +127,24 @@ func TestLatestPredecessorHistory(t *testing.T) { v: "v23.1.1", k: 3, expectedLatest: []string{"19.2.0", "22.1.12", "22.2.8"}, - expectedRandom: []string{"19.2.0", "22.1.12", "22.2.1"}, + expectedRandom: []string{"19.2.0", "22.1.8", "22.2.8"}, }, { name: "with pre-release", v: "v23.1.1-beta.1", k: 2, expectedLatest: []string{"22.1.12", "22.2.8"}, - expectedRandom: []string{"22.1.0", "22.2.5"}, + expectedRandom: []string{"22.1.8", "22.2.8"}, }, } - for _, tc := range testCases { - oldReleaseData := releaseData - releaseData = testReleaseData - defer func() { releaseData = oldReleaseData }() + oldReleaseData := releaseData + releaseData = testReleaseData + defer func() { releaseData = oldReleaseData }() + for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { + resetRNG() // deterministic results latestHistory, latestErr := LatestPredecessorHistory(version.MustParse(tc.v), tc.k) randomHistory, randomErr := RandomPredecessorHistory(rng, version.MustParse(tc.v), tc.k) if tc.expectedErr == "" {