From 69ecb76c34948d657721ca14e2d44edd0a0a3cf4 Mon Sep 17 00:00:00 2001 From: Evan Wall Date: Fri, 8 Sep 2023 09:56:54 -0400 Subject: [PATCH 1/3] sql: rename scheduleID to scheduleIDIdx and createStmt to createStmtIdx scheduleID and createStmt indexes used to populate slice literals for show results. Rename these to avoid accidental usage and to prevent shadowing in the sql package. Release note: None --- pkg/sql/show_create_external_connection.go | 4 ++-- pkg/sql/show_create_schedule.go | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/sql/show_create_external_connection.go b/pkg/sql/show_create_external_connection.go index 086cc6f8e956..24c710f4f20f 100644 --- a/pkg/sql/show_create_external_connection.go +++ b/pkg/sql/show_create_external_connection.go @@ -120,8 +120,8 @@ func (p *planner) ShowCreateExternalConnection( var rows []tree.Datums for _, conn := range connections { row := tree.Datums{ - scheduleID: tree.NewDString(conn.ConnectionName()), - createStmt: tree.NewDString(conn.UnredactedConnectionStatement()), + scheduleIDIdx: tree.NewDString(conn.ConnectionName()), + createStmtIdx: tree.NewDString(conn.UnredactedConnectionStatement()), } rows = append(rows, row) } diff --git a/pkg/sql/show_create_schedule.go b/pkg/sql/show_create_schedule.go index ddc45639882c..542042433e67 100644 --- a/pkg/sql/show_create_schedule.go +++ b/pkg/sql/show_create_schedule.go @@ -33,8 +33,8 @@ var showCreateTableColumns = colinfo.ResultColumns{ } const ( - scheduleID = iota - createStmt + scheduleIDIdx = iota + createStmtIdx ) func loadSchedules(params runParams, n *tree.ShowCreateSchedules) ([]*jobs.ScheduledJob, error) { @@ -126,8 +126,8 @@ func (p *planner) ShowCreateSchedule( } row := tree.Datums{ - scheduleID: tree.NewDInt(tree.DInt(sj.ScheduleID())), - createStmt: tree.NewDString(createStmtStr), + scheduleIDIdx: tree.NewDInt(tree.DInt(sj.ScheduleID())), + createStmtIdx: tree.NewDString(createStmtStr), } rows = append(rows, row) } From fb43087f4905121d7dc2c0bf70f856fca7b0841e Mon Sep 17 00:00:00 2001 From: Evan Wall Date: Fri, 8 Sep 2023 11:22:59 -0400 Subject: [PATCH 2/3] ttl: refactor row_level_ttl logic tests Split large subtests into smaller subtests to make debugging easier. Release note: None --- .../testdata/logic_test/row_level_ttl | 162 ++++++++++++++---- 1 file changed, 127 insertions(+), 35 deletions(-) diff --git a/pkg/sql/logictest/testdata/logic_test/row_level_ttl b/pkg/sql/logictest/testdata/logic_test/row_level_ttl index 88c0e2c8773b..5d33b6d84fe1 100644 --- a/pkg/sql/logictest/testdata/logic_test/row_level_ttl +++ b/pkg/sql/logictest/testdata/logic_test/row_level_ttl @@ -503,27 +503,31 @@ ALTER TABLE test.public.tbl_ttl_on_noop WITH (ttl = 'on', ...) subtest end -subtest ttl_job_cron +subtest ttl_job_cron_invalid statement error invalid cron expression for "ttl_job_cron" CREATE TABLE tbl () WITH (ttl_expire_after = '10 seconds', ttl_job_cron = 'bad expr') +subtest end + +subtest create_ttl_job_cron + statement ok -CREATE TABLE tbl_ttl_job_cron ( +CREATE TABLE tbl_create_ttl_job_cron ( id INT PRIMARY KEY ) WITH (ttl_expire_after = '10 minutes', ttl_job_cron = '@daily') query T -SELECT create_statement FROM [SHOW CREATE TABLE tbl_ttl_job_cron] +SELECT create_statement FROM [SHOW CREATE TABLE tbl_create_ttl_job_cron] ---- -CREATE TABLE public.tbl_ttl_job_cron ( +CREATE TABLE public.tbl_create_ttl_job_cron ( 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 tbl_ttl_job_cron_pkey PRIMARY KEY (id ASC) + CONSTRAINT tbl_create_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' +SELECT oid FROM pg_class WHERE relname = 'tbl_create_ttl_job_cron' query TTT SELECT schedule_status, recurrence, owner FROM [SHOW SCHEDULES] @@ -531,18 +535,29 @@ WHERE label = 'row-level-ttl-$table_id' ---- ACTIVE @daily root +subtest end + +subtest set_ttl_job_cron + statement ok -ALTER TABLE tbl_ttl_job_cron SET (ttl_job_cron = '@weekly') +CREATE TABLE tbl_set_ttl_job_cron ( + id INT PRIMARY KEY +) WITH (ttl_expire_after = '10 minutes', ttl_job_cron = '@daily') + +statement ok +ALTER TABLE tbl_set_ttl_job_cron SET (ttl_job_cron = '@weekly') query T -SELECT create_statement FROM [SHOW CREATE TABLE tbl_ttl_job_cron] +SELECT create_statement FROM [SHOW CREATE TABLE tbl_set_ttl_job_cron] ---- -CREATE TABLE public.tbl_ttl_job_cron ( +CREATE TABLE public.tbl_set_ttl_job_cron ( 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 tbl_ttl_job_cron_pkey PRIMARY KEY (id ASC) + CONSTRAINT tbl_set_ttl_job_cron_pkey PRIMARY KEY (id ASC) ) WITH (ttl = 'on', ttl_expire_after = '00:10:00':::INTERVAL, ttl_job_cron = '@weekly') +let $table_id +SELECT oid FROM pg_class WHERE relname = 'tbl_set_ttl_job_cron' query TTT SELECT schedule_status, recurrence, owner FROM [SHOW SCHEDULES] @@ -550,18 +565,30 @@ WHERE label = 'row-level-ttl-$table_id' ---- ACTIVE @weekly root +subtest end + +subtest reset_ttl_job_cron + +statement ok +CREATE TABLE tbl_reset_ttl_job_cron ( + id INT PRIMARY KEY +) WITH (ttl_expire_after = '10 minutes', ttl_job_cron = '@daily') + statement ok -ALTER TABLE tbl_ttl_job_cron RESET (ttl_job_cron) +ALTER TABLE tbl_reset_ttl_job_cron RESET (ttl_job_cron) query T -SELECT create_statement FROM [SHOW CREATE TABLE tbl_ttl_job_cron] +SELECT create_statement FROM [SHOW CREATE TABLE tbl_reset_ttl_job_cron] ---- -CREATE TABLE public.tbl_ttl_job_cron ( +CREATE TABLE public.tbl_reset_ttl_job_cron ( 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 tbl_ttl_job_cron_pkey PRIMARY KEY (id ASC) + CONSTRAINT tbl_reset_ttl_job_cron_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_reset_ttl_job_cron' + query TTT SELECT schedule_status, recurrence, owner FROM [SHOW SCHEDULES] WHERE label = 'row-level-ttl-$table_id' @@ -810,19 +837,14 @@ CREATE TABLE public.tbl_add_ttl_expiration_expression_to_ttl_expire_after ( FAMILY fam_0_id_expire_at_crdb_internal_expiration (id, expire_at, crdb_internal_expiration) ) WITH (ttl = 'on', ttl_expire_after = '00:10:00':::INTERVAL, ttl_expiration_expression = 'crdb_internal_expiration', ttl_job_cron = '@hourly') -statement ok -ALTER TABLE tbl_add_ttl_expiration_expression_to_ttl_expire_after RESET (ttl_expiration_expression) +let $table_id +SELECT oid FROM pg_class WHERE relname = 'tbl_add_ttl_expiration_expression_to_ttl_expire_after' -query T -SELECT create_statement FROM [SHOW CREATE TABLE tbl_add_ttl_expiration_expression_to_ttl_expire_after] +query TTT +SELECT schedule_status, recurrence, owner FROM [SHOW SCHEDULES] +WHERE label = 'row-level-ttl-$table_id' ---- -CREATE TABLE public.tbl_add_ttl_expiration_expression_to_ttl_expire_after ( - id INT8 NOT NULL, - expire_at TIMESTAMPTZ 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_add_ttl_expiration_expression_to_ttl_expire_after_pkey PRIMARY KEY (id ASC), - FAMILY fam_0_id_expire_at_crdb_internal_expiration (id, expire_at, crdb_internal_expiration) -) WITH (ttl = 'on', ttl_expire_after = '00:10:00':::INTERVAL, ttl_job_cron = '@hourly') +ACTIVE @hourly root subtest end @@ -849,18 +871,14 @@ CREATE TABLE public.tbl_add_ttl_expire_after_to_ttl_expiration_expression ( FAMILY fam_0_id_expire_at (id, expire_at, crdb_internal_expiration) ) WITH (ttl = 'on', ttl_expire_after = '00:10:00':::INTERVAL, ttl_expiration_expression = 'expire_at', ttl_job_cron = '@hourly') -statement ok -ALTER TABLE tbl_add_ttl_expire_after_to_ttl_expiration_expression RESET (ttl_expire_after) +let $table_id +SELECT oid FROM pg_class WHERE relname = 'tbl_add_ttl_expire_after_to_ttl_expiration_expression' -query T -SELECT create_statement FROM [SHOW CREATE TABLE tbl_add_ttl_expire_after_to_ttl_expiration_expression] +query TTT +SELECT schedule_status, recurrence, owner FROM [SHOW SCHEDULES] +WHERE label = 'row-level-ttl-$table_id' ---- -CREATE TABLE public.tbl_add_ttl_expire_after_to_ttl_expiration_expression ( - id INT8 NOT NULL, - expire_at TIMESTAMPTZ NULL, - CONSTRAINT tbl_add_ttl_expire_after_to_ttl_expiration_expression_pkey PRIMARY KEY (id ASC), - FAMILY fam_0_id_expire_at (id, expire_at) -) +ACTIVE @hourly root subtest end @@ -1089,3 +1107,77 @@ statement ok DROP TABLE "Table-Name" subtest end + +# sets both ttl_exipire_after and ttl_expiration_expression +subtest set_both_reset_ttl_expire_after + +statement ok +CREATE TABLE tbl_set_both_reset_ttl_expire_after ( + id INT PRIMARY KEY, + expire_at TIMESTAMPTZ, + FAMILY (id, expire_at) +) WITH ( + ttl_expire_after = '10m', + ttl_expiration_expression = 'expire_at' +) + +statement ok +ALTER TABLE tbl_set_both_reset_ttl_expire_after RESET (ttl_expire_after) + +query T +SELECT create_statement FROM [SHOW CREATE TABLE tbl_set_both_reset_ttl_expire_after] +---- +CREATE TABLE public.tbl_set_both_reset_ttl_expire_after ( + id INT8 NOT NULL, + expire_at TIMESTAMPTZ NULL, + CONSTRAINT tbl_set_both_reset_ttl_expire_after_pkey PRIMARY KEY (id ASC), + FAMILY fam_0_id_expire_at_crdb_internal_expiration (id, expire_at) +) + +let $table_id +SELECT oid FROM pg_class WHERE relname = 'tbl_set_both_reset_ttl_expire_after' + +query TTT +SELECT schedule_status, recurrence, owner FROM [SHOW SCHEDULES] +WHERE label = 'row-level-ttl-$table_id' +---- + +subtest end + +# sets both ttl_exipire_after and ttl_expiration_expression +subtest set_both_reset_ttl_expiration_expression + +statement ok +CREATE TABLE tbl_set_both_reset_ttl_expiration_expression ( + id INT PRIMARY KEY, + expire_at TIMESTAMPTZ, + FAMILY (id, expire_at) +) WITH ( + ttl_expire_after = '10m', + ttl_expiration_expression = 'expire_at' +) + +statement ok +ALTER TABLE tbl_set_both_reset_ttl_expiration_expression RESET (ttl_expiration_expression) + +query T +SELECT create_statement FROM [SHOW CREATE TABLE tbl_set_both_reset_ttl_expiration_expression] +---- +CREATE TABLE public.tbl_set_both_reset_ttl_expiration_expression ( + id INT8 NOT NULL, + expire_at TIMESTAMPTZ 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_set_both_reset_ttl_expiration_expression_pkey PRIMARY KEY (id ASC), + FAMILY fam_0_id_expire_at_crdb_internal_expiration (id, expire_at, crdb_internal_expiration) +) 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_set_both_reset_ttl_expiration_expression' + +query TTT +SELECT schedule_status, recurrence, owner FROM [SHOW SCHEDULES] +WHERE label = 'row-level-ttl-$table_id' +---- +ACTIVE @hourly root + +subtest end From 2de77b1f9d8991a48a1bf5a0d8bead96a9e7acd8 Mon Sep 17 00:00:00 2001 From: Evan Wall Date: Thu, 7 Sep 2023 10:54:36 -0400 Subject: [PATCH 3/3] ttl: do not remove all table TTL settings on RESET (ttl_expire_after) Fixes #109138 Previously running `RESET (ttl_expire_after)` would remove all table TTL settings if `ttl_expiration_expression` was set. This PR fixes the schema changer to only remove all TTL settings on `RESET (ttl)`. Release note (bug fix): `RESET (ttl_expire_after)` no longer incorrectly removes `ttl_expiration_expression`. --- pkg/sql/alter_table.go | 36 ++++++++----------- .../testdata/logic_test/row_level_ttl | 3 +- pkg/sql/schema_changer.go | 2 +- 3 files changed, 18 insertions(+), 23 deletions(-) diff --git a/pkg/sql/alter_table.go b/pkg/sql/alter_table.go index 797a25af881a..baa53a30f3ce 100644 --- a/pkg/sql/alter_table.go +++ b/pkg/sql/alter_table.go @@ -1952,14 +1952,8 @@ func handleTTLStorageParamChange( } } - // Add TTL mutation if TTL is newly SET. - addTTLMutation := before == nil && after != nil - // Create new column. if (before == nil || !before.HasDurationExpr()) && (after != nil && after.HasDurationExpr()) { - // Adding a TTL requires adding the automatic column and deferring the TTL - // addition to after the column is successfully added. - addTTLMutation = true if _, err := tableDesc.FindColumnWithName(catpb.TTLDefaultExpirationColumnName); err == nil { return false, pgerror.Newf( pgcode.InvalidTableDefinition, @@ -1994,21 +1988,9 @@ func handleTTLStorageParamChange( } } - // Add TTL mutation so that job is scheduled in SchemaChanger. - if addTTLMutation { - tableDesc.AddModifyRowLevelTTLMutation( - &descpb.ModifyRowLevelTTL{RowLevelTTL: after}, - descpb.DescriptorMutation_ADD, - ) - } - - dropTTLMutation := before != nil && after == nil - // Remove existing column. if (before != nil && before.HasDurationExpr()) && (after == nil || !after.HasDurationExpr()) { telemetry.Inc(sqltelemetry.RowLevelTTLDropped) - // Create the DROP COLUMN job and the associated mutation. - dropTTLMutation = true droppedViews, err := dropColumnImpl(params, tn, tableDesc, after, &tree.AlterTableDropColumn{ Column: catpb.TTLDefaultExpirationColumnName, }) @@ -2021,10 +2003,22 @@ func handleTTLStorageParamChange( } } - if dropTTLMutation { + // Adding TTL requires adding the TTL job before adding the TTL fields. + // Removing TTL requires removing the TTL job before removing the TTL fields. + var direction descpb.DescriptorMutation_Direction + switch { + case before == nil && after != nil: + direction = descpb.DescriptorMutation_ADD + case before != nil && after == nil: + direction = descpb.DescriptorMutation_DROP + default: + descriptorChanged = true + } + if !descriptorChanged { + // Add TTL mutation so that job is scheduled in SchemaChanger. tableDesc.AddModifyRowLevelTTLMutation( &descpb.ModifyRowLevelTTL{RowLevelTTL: after}, - descpb.DescriptorMutation_DROP, + direction, ) } @@ -2035,7 +2029,7 @@ func handleTTLStorageParamChange( } } - descriptorChanged = !addTTLMutation && !dropTTLMutation + // Modify the TTL fields here because it will not be done in a mutation. if descriptorChanged { tableDesc.RowLevelTTL = after } diff --git a/pkg/sql/logictest/testdata/logic_test/row_level_ttl b/pkg/sql/logictest/testdata/logic_test/row_level_ttl index 5d33b6d84fe1..54c3a674ec6d 100644 --- a/pkg/sql/logictest/testdata/logic_test/row_level_ttl +++ b/pkg/sql/logictest/testdata/logic_test/row_level_ttl @@ -1132,7 +1132,7 @@ CREATE TABLE public.tbl_set_both_reset_ttl_expire_after ( expire_at TIMESTAMPTZ NULL, CONSTRAINT tbl_set_both_reset_ttl_expire_after_pkey PRIMARY KEY (id ASC), FAMILY fam_0_id_expire_at_crdb_internal_expiration (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 = 'tbl_set_both_reset_ttl_expire_after' @@ -1141,6 +1141,7 @@ query TTT SELECT schedule_status, recurrence, owner FROM [SHOW SCHEDULES] WHERE label = 'row-level-ttl-$table_id' ---- +ACTIVE @hourly root subtest end diff --git a/pkg/sql/schema_changer.go b/pkg/sql/schema_changer.go index 0da74d3db2fe..ea70dd6e35c4 100644 --- a/pkg/sql/schema_changer.go +++ b/pkg/sql/schema_changer.go @@ -1609,7 +1609,7 @@ func (sc *SchemaChanger) done(ctx context.Context) error { return err } } - scTable.RowLevelTTL = nil + scTable.RowLevelTTL = modify.RowLevelTTL() } }