Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ttl: Schedule TTL job when ttl_expiration_expression is set after table creation #88260

Merged
merged 1 commit into from
Sep 25, 2022
Merged

ttl: Schedule TTL job when ttl_expiration_expression is set after table creation #88260

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 37 additions & 29 deletions pkg/sql/alter_table.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -745,10 +745,8 @@ func (n *alterTableNode) startExec(params runParams) error {
return err
}

descriptorChanged = true

var err error
err = handleTTLStorageParamChange(
descriptorChanged, err = handleTTLStorageParamChange(
params,
tn,
setter.TableDesc,
Expand All @@ -767,10 +765,9 @@ func (n *alterTableNode) startExec(params runParams) error {
); err != nil {
return err
}
descriptorChanged = true

var err error
err = handleTTLStorageParamChange(
descriptorChanged, err = handleTTLStorageParamChange(
params,
tn,
setter.TableDesc,
Expand Down Expand Up @@ -1847,7 +1844,7 @@ func dropColumnImpl(

func handleTTLStorageParamChange(
params runParams, tn *tree.TableName, tableDesc *tabledesc.Mutable, after *catpb.RowLevelTTL,
) error {
) (descriptorChanged bool, err error) {

before := tableDesc.GetRowLevelTTL()

Expand All @@ -1865,25 +1862,25 @@ func handleTTLStorageParamChange(
params.p.txn,
)
if err != nil {
return err
return false, err
}
if err := s.SetSchedule(after.DeletionCronOrDefault()); err != nil {
return err
return false, err
}
if err := s.Update(params.ctx, params.ExecCfg().InternalExecutor, params.p.txn); err != nil {
return err
return false, err
}
}

// Update default expression on automated column if required.
if before.HasDurationExpr() && after.HasDurationExpr() && before.DurationExpr != after.DurationExpr {
col, err := tableDesc.FindColumnWithName(colinfo.TTLDefaultExpirationColumnName)
if err != nil {
return err
return false, err
}
intervalExpr, err := parser.ParseExpr(string(after.DurationExpr))
if err != nil {
return errors.Wrapf(err, "unexpected expression for TTL duration")
return false, errors.Wrapf(err, "unexpected expression for TTL duration")
}
newExpr := rowLevelTTLAutomaticColumnExpr(intervalExpr)

Expand All @@ -1895,7 +1892,7 @@ func handleTTLStorageParamChange(
&col.ColumnDesc().DefaultExpr,
"TTL DEFAULT",
); err != nil {
return err
return false, err
}

if err := updateNonComputedColExpr(
Expand All @@ -1906,27 +1903,29 @@ func handleTTLStorageParamChange(
&col.ColumnDesc().OnUpdateExpr,
"TTL UPDATE",
); err != nil {
return err
return false, err
}
}
}

hasTTLMutation := false
// 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(colinfo.TTLDefaultExpirationColumnName); err == nil {
return pgerror.Newf(
return false, 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
return false, err
}
addCol := &tree.AlterTableAddColumn{
ColumnDef: col,
Expand All @@ -1943,52 +1942,61 @@ func handleTTLStorageParamChange(
tableDesc,
addCol,
); err != nil {
return err
return false, err
}
version := params.ExecCfg().Settings.Version.ActiveVersion(params.ctx)
if err := tableDesc.AllocateIDs(params.ctx, version); err != nil {
return false, err
}
}

// Add TTL mutation so that job is scheduled in SchemaChanger.
if addTTLMutation {
tableDesc.AddModifyRowLevelTTLMutation(
&descpb.ModifyRowLevelTTL{RowLevelTTL: after},
descpb.DescriptorMutation_ADD,
)
hasTTLMutation = true
version := params.ExecCfg().Settings.Version.ActiveVersion(params.ctx)
if err := tableDesc.AllocateIDs(params.ctx, version); err != nil {
return err
}
}

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: colinfo.TTLDefaultExpirationColumnName,
})
if err != nil {
return err
return false, err
}
// This should never happen as we do not CASCADE, but error again just in case.
if len(droppedViews) > 0 {
return pgerror.Newf(pgcode.InvalidParameterValue, "cannot drop TTL automatic column if it is depended on by a view")
return false, pgerror.Newf(pgcode.InvalidParameterValue, "cannot drop TTL automatic column if it is depended on by a view")
}
}

if dropTTLMutation {
tableDesc.AddModifyRowLevelTTLMutation(
&descpb.ModifyRowLevelTTL{RowLevelTTL: before},
&descpb.ModifyRowLevelTTL{RowLevelTTL: after},
descpb.DescriptorMutation_DROP,
)
hasTTLMutation = true
}

// Validate the type and volatility of ttl_expiration_expression.
if after != nil {
if err := schemaexpr.ValidateTTLExpirationExpression(params.ctx, tableDesc, params.p.SemaCtx(), tn, after); err != nil {
return err
return false, err
}
}

if !hasTTLMutation {
descriptorChanged = !addTTLMutation && !dropTTLMutation
if descriptorChanged {
tableDesc.RowLevelTTL = after
}

return nil
return descriptorChanged, nil
}

// tryRemoveFKBackReferences determines whether the provided unique constraint
Expand Down
5 changes: 2 additions & 3 deletions pkg/sql/create_table.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1333,9 +1333,7 @@ func NewTableDesc(
); err != nil {
return nil, err
}
if updatedRowLevelTTL := setter.UpdatedRowLevelTTL; updatedRowLevelTTL != nil {
setter.TableDesc.RowLevelTTL = updatedRowLevelTTL
}
setter.TableDesc.RowLevelTTL = setter.UpdatedRowLevelTTL

indexEncodingVersion := descpb.StrictIndexColumnIDGuaranteesVersion
isRegionalByRow := n.Locality != nil && n.Locality.LocalityLevel == tree.LocalityLevelRow
Expand Down Expand Up @@ -1491,6 +1489,7 @@ func NewTableDesc(
hasRowLevelTTLColumn = true
break
}

}
}
if !hasRowLevelTTLColumn {
Expand Down
115 changes: 102 additions & 13 deletions pkg/sql/logictest/testdata/logic_test/row_level_ttl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,47 @@
subtest todo_add_subtests
subtest ttl_expire_after_must_be_interval

statement error value of "ttl_expire_after" must be an interval
CREATE TABLE tbl (id INT PRIMARY KEY, text TEXT) WITH (ttl_expire_after = ' xx invalid interval xx')

subtest end

subtest ttl_expire_after_must_be_at_least_zero

statement error value of "ttl_expire_after" must be at least zero
CREATE TABLE tbl (id INT PRIMARY KEY, text TEXT) WITH (ttl_expire_after = '-10 minutes')

subtest end

subtest ttl_expiration_expression_must_be_string

statement error parameter "ttl_expiration_expression" requires a string value
CREATE TABLE tbl (id INT PRIMARY KEY, text TEXT) WITH (ttl_expiration_expression = 0)

subtest end

subtest ttl_expiration_expression_must_be_valid_expression

statement error ttl_expiration_expression "; DROP DATABASE defaultdb" must be a valid expression: at or near "EOF": syntax error
CREATE TABLE tbl (id INT PRIMARY KEY, text TEXT) WITH (ttl_expiration_expression = '; DROP DATABASE defaultdb')

subtest end

subtest ttl_expiration_expression_must_be_timestamptz

statement error expected ttl_expiration_expression expression to have type timestamptz, but 'text' has type string
CREATE TABLE tbl (id INT PRIMARY KEY, text TEXT) WITH (ttl_expiration_expression = 'text')

subtest end

subtest ttl_expire_after_or_ttl_expiration_expression_must_be_set

statement error "ttl_expire_after" and/or "ttl_expiration_expression" must be set
CREATE TABLE tbl (id INT PRIMARY KEY, text TEXT) WITH (ttl = 'on')

subtest end

subtest todo_add_subtests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this about?

Copy link
Contributor Author

@ecwall ecwall Sep 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was isolating the failing tests to debug by putting them in subtests, but I didn't do all of them so I just moved subtest todo_add_subtests from above.


query T noticetrace
CREATE TABLE tbl_ttl_automatic_column (id INT PRIMARY KEY, text TEXT) WITH (ttl_automatic_column = 'on')
----
Expand Down Expand Up @@ -875,21 +899,35 @@ statement ok
ROLLBACK

statement ok
ALTER TABLE tbl SET (ttl_expire_after = '10 minutes', ttl_select_batch_size = 200)
DROP TABLE tbl

subtest end

subtest create_table_no_ttl_set_ttl_expire_after
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the existing test to a subtest so I can run it individually.


statement ok
CREATE TABLE create_table_no_ttl_set_ttl_expire_after (
id INT PRIMARY KEY,
text TEXT,
FAMILY (id, text)
)

statement ok
ALTER TABLE create_table_no_ttl_set_ttl_expire_after SET (ttl_expire_after = '10 minutes')

query T
SELECT create_statement FROM [SHOW CREATE TABLE tbl]
SELECT create_statement FROM [SHOW CREATE TABLE create_table_no_ttl_set_ttl_expire_after]
----
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 (id, text, crdb_internal_expiration)
) WITH (ttl = 'on', ttl_expire_after = '00:10:00':::INTERVAL, ttl_job_cron = '@hourly', ttl_select_batch_size = 200)
CREATE TABLE public.create_table_no_ttl_set_ttl_expire_after (
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 create_table_no_ttl_set_ttl_expire_after_pkey PRIMARY KEY (id ASC),
FAMILY fam_0_id_text (id, text, 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'
SELECT oid FROM pg_class WHERE relname = 'create_table_no_ttl_set_ttl_expire_after'

query TTT
SELECT schedule_status, recurrence, owner FROM [SHOW SCHEDULES]
Expand All @@ -898,9 +936,60 @@ WHERE label = 'row-level-ttl-$table_id'
ACTIVE @hourly root

statement ok
DROP TABLE tbl
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'
----

subtest end

subtest create_table_no_ttl_set_ttl_expiration_expression
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the existing test but uses ttl_expiration_expression instead of ttl_expire_after.


statement ok
CREATE TABLE create_table_no_ttl_set_ttl_expiration_expression (
id INT PRIMARY KEY,
text TEXT,
expire_at TIMESTAMPTZ,
FAMILY (id, text)
)

statement ok
ALTER TABLE create_table_no_ttl_set_ttl_expiration_expression SET (ttl_expiration_expression = 'expire_at')

query T
SELECT create_statement FROM [SHOW CREATE TABLE create_table_no_ttl_set_ttl_expiration_expression]
----
CREATE TABLE public.create_table_no_ttl_set_ttl_expiration_expression (
id INT8 NOT NULL,
text STRING NULL,
expire_at TIMESTAMPTZ NULL,
CONSTRAINT create_table_no_ttl_set_ttl_expiration_expression_pkey PRIMARY KEY (id ASC),
FAMILY fam_0_id_text_expire_at (id, text, 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'

query TTT
SELECT schedule_status, recurrence, owner FROM [SHOW SCHEDULES]
WHERE label = 'row-level-ttl-$table_id'
----
ACTIVE @hourly root
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This query returns 0 results without the alter_table.go change.


statement ok
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'
----

subtest end

subtest special_table_name

# Special table name.
statement ok
CREATE TABLE "Table-Name" (id INT PRIMARY KEY) WITH (ttl_expire_after = '10 hours')

Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/schema_changer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1513,7 +1513,7 @@ func (sc *SchemaChanger) done(ctx context.Context) error {

// If we are modifying TTL, then make sure the schedules are created
// or dropped as appropriate.
if modify := m.AsModifyRowLevelTTL(); modify != nil {
if modify := m.AsModifyRowLevelTTL(); modify != nil && !modify.IsRollback() {
if fn := sc.testingKnobs.RunBeforeModifyRowLevelTTL; fn != nil {
if err := fn(); err != nil {
return err
Expand Down
Loading