sql: implement ADD/DROP schema changes for row-level TTL #76216

otan · 2022-02-08T04:13:01Z

These commits adds the "scarier" part of the row level TTL job, in particular:

DROP TABLE/DATABASE/SCHEMA (this required adding it to the declarative schema changer as well, see the last two commits)
SET (ttl) for the first time, which adds the automatic column
RESET (ttl), which drops the automatic column

This is handled in both the old and new schema changer.

cockroach-teamcity · 2022-02-08T04:13:17Z

This change is

otan · 2022-02-08T10:37:59Z

my reading of the new schema changer code is that I "only" need to support DROP TABLE/DROP DATABASE/DROP SCHEMA. i'll look into that tomorrow.

the way add/drop column is being used here is via ALTER TABLE ... SET/RESET, which means it would use the original schema changer anyway. but i'm not sure how "mixing" supported and unsupported statements works.

postamar

Thanks for putting this up for review so quickly. I think it's a good, solid change overall, as far as I can tell (I don't know much about scheduled jobs). The drop column is dodgy of course (not your doing, it always was dodgy) and that's exactly the kind of thing the declarative schema changer will be able to help with.

I do have a couple of high level comments and questions:

This is missing table descriptor validation checks for the TTL descriptor. I don't think they can wait until later. Every time you add a field in the TTL proto, you should have a corresponding validation check and unit tests in tabledesc. This will strengthen all of your tests so, so much.
Wouldn't it be possible to do without the new mutation types? It feels like the TTL descriptor could simply have some kind of status enum indicating whether the schedule:
a. needs to be updated,
b. needs to be deleted,
c. no-op.

ccing @ajwerner for his 👀 on this last one, he might want to weigh in on whether this is a good idea or not.

Reviewed 3 of 3 files at r1, 14 of 14 files at r2, 9 of 9 files at r3, 6 of 6 files at r4, 5 of 5 files at r5, 2 of 2 files at r6, 3 of 3 files at r7, 3 of 3 files at r8, 11 of 11 files at r9, 2 of 2 files at r10, 5 of 5 files at r11, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @otan)

pkg/sql/alter_table.go, line 1765 at r7 (raw file):

		// Do not have to do anything here.
	case before != nil && after != nil:
		if before.DeletionCron != after.DeletionCron {

What if before you have "" and after you have "@hourly"? Do we care about those kinds of no-ops?

pkg/sql/create_table.go, line 2342 at r5 (raw file):

// defaultTTLScheduleCron is the default cron duration for row-level TTL.
// defaultTTLScheduleCron cannot be a cluster setting as this would involve
// changing all existing schedules to match the new setting.

Would it make sense to enforce the deletion_cron field in the descriptor to be non-empty instead? I won't insist on it, but it feels like that would be more robust.

pkg/sql/schema_changer.go, line 388 at r8 (raw file):

	if tableDesc.Dropped() && tableDesc.GetRowLevelTTL() != nil {
		if err := sc.db.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
			scheduleID := tableDesc.GetRowLevelTTL().ScheduleID

Is this guaranteed to be non-zero at this point?

pkg/sql/schema_changer.go, line 1222 at r11 (raw file):

						}
						scTable.RowLevelTTL.ScheduleID = j.ScheduleID()
					}

nit: shouldn't this have been in an earlier commit?

pkg/sql/show_create.go, line 195 at r5 (raw file):

		}
		if cron := ttl.DeletionCron; cron != "" {
			storageParams = append(storageParams, fmt.Sprintf(`ttl_delete_batch_size = '%s'`, cron))

Wrong format string. I expect this will show up in tests also.

pkg/sql/catalog/descpb/structured.proto, line 612 at r9 (raw file):

  optional TableDescriptor.RowLevelTTL row_level_ttl = 1 [(gogoproto.customname) = "RowLevelTTL"];
}

Is this message likely to have more fields added to it? It doesn't seem necessary, to be honest.

pkg/sql/schema_changer_test.go, line 7516 at r9 (raw file):

			setup: `
			CREATE DATABASE t;
			USE t;

FYI, these USE t don't seem to be necessary. I don't care much either way.

otan · 2022-02-08T22:06:19Z

Wouldn't it be possible to do without the new mutation types? It feels like the TTL descriptor could simply have some kind of status enum indicating whether the schedule:
a. needs to be updated,
b. needs to be deleted,
c. no-op.

i can see this getting complicated because if something fails in between the reverse mutations will also need to dive deeper into the tabledescriptor proto and zero things out, but only on add column / drop column if it matches a certain column name.

for me, the separate mutation feels cleaner, but yeah more input would be good. i'll hold off on making that change (as it is not trivial :) ) for now

postamar · 2022-02-09T00:33:19Z

The way I understand it, mutations represent a future change to the table descriptor itself. It seemed to me that the TTL descriptor was effectively the source of truth for something not in the table descriptor, so to me it didn't fit into that mold. Of course, if there's subtle interplay with adding/removing the ttl column, that would invalidate my argument. In any case having a new type of mutation isn't the worst thing in the world but if it's avoidable it's better.

otan

This is missing table descriptor validation checks for the TTL descriptor

done.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @hourly and @postamar)

pkg/sql/alter_table.go, line 1765 at r7 (raw file):