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

[ecwall]

fixes #88254
refs #88560

Previously a TTL mutation was added for the schema changer only when
ttl_expire_after was set. Now a TTL mutation is also added when
ttl_expiration_expression is set.

Release note: None

[cockroach-teamcity]

This change is 

[ecwall]

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

[ecwall]

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

[ecwall]

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

[otan]

going to defer to schema to take a look at this one!

[otan]

couple of notes in my head

• adding a mutation seems like overkill in this case - if that fails, the TTL remains on the table but has no job in that case. it may be better to just add the schedule.
• make sure dropping the ttl_expiration_expression also drops the scheduled job

[ecwall]

couple of notes in my head

• adding a mutation seems like overkill in this case - if that fails, the TTL remains on the table but has no job in that case. it may be better to just add the schedule.
• make sure dropping the ttl_expiration_expression also drops the scheduled job

Can we simplify the process and make maybeUpdateScheduledJobsForRowLevelTTL always schedule or unschedule the job then and never use a mutation?

[otan]

as long as cancelling the job adding or dropping the TTL column also correctly accounts for the schedule creation

[ecwall]

as long as cancelling the job adding or dropping the TTL column also correctly accounts for the schedule creation

Can TableDescriptor.RowLevelTTL.ScheduleID be moved to TableDescriptor.TTLScheduleID since it looks like it is not modified with TableDescriptor.RowLevelTTL in the same transaction?

The previous ScheduleID needs to be maintained in the table descriptor until the job is unscheduled if TTL is unset on the table.

[ajwerner]

My problem, from looking at this, is that I don't have a well-formed mental model on how this stuff is supposed to work. @otan I guess I wonder why you think a mutation is in any way wrong here? It seems somewhat important to me.

Consider:

CREATE TABLE t (
   id INT PRIMARY KEY,
   text STRING
   expire_at TIMESTAMPTZ,
   FAMILY (id, text, expire_at)
);

INSERT INTO t VALUES (1, 'a', NULL), (2, 'a', NULL);
BEGIN;
CREATE UNIQUE INDEX idx ON t(text);
ALTER TABLE create_table_no_ttl_set_ttl_expiration_expression SET (ttl_expiration_expression = 'expire_at');
COMMIT;

What should happen? I sincerely hope we'd roll back the ttl change.

[otan]

ah i think the code as is immediately sets TTLExpirationExpression on the TableDescriptor. if we moved the TTLExpirationExpression to be in mutation and only apply the TableDescriptor change on the schema changer when processing the mutation then i think we're in the clear

i am totally rusty after 6 months 😬

[ajwerner]

it's an edge case but what happens if you have multiple ALTER TABLE commands which muck with row-level TTL settings in the same transaction? My reading is that it's just going to add more mutations. Is that the right thing?

Generally this seems sane to me. Thanks for tracking it down and picking up all the context you did on this code.

[ajwerner]

what is this about?

[ecwall]

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.

[ajwerner]

nit: name the return bool

[ecwall]

it's an edge case but what happens if you have multiple ALTER TABLE commands which muck with row-level TTL settings in the same transaction? My reading is that it's just going to add more mutations. Is that the right thing?

Generally this seems sane to me. Thanks for tracking it down and picking up all the context you did on this code.

Right now it adds mutations if the job is scheduled/unscheduled and if the column is added/dropped, otherwise it does the change immediately. I have another followup PR to add tests for the immediate vs mutation changes and I can add tests for this also.

[ajwerner]

LGTM

[ecwall]

bors r=ajwerner

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)