changefeedccl: protect system.comments and system.zones

[stevendanna]

The catalog reader reads from system.descriptor, system.comments, and system.zones when reading a table descriptor from disk. We were only protecting system.descriptors.

Further, the test here previously wasn't testing anything because it was only forcing the GC-threshold to 1 second before read timestamp it was using.

Informs #128806

Release note (bug fix): Fix bug that could prevent a CHANGEFEED from being able to resume after being paused for prolonged period of time.

[cockroach-teamcity]

This change is 

[andyyang890]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @stevendanna)

---

pkg/ccl/changefeedccl/protected_timestamps.go line 49 at r1 (raw file):

	// We protect system.descriptors because a changefeed needs all of the history
	// of table descriptors to version data.
	tablesToProtect := make(descpb.IDs, 0, targets.NumUniqueTables()+1)

fyi might want to consider updating this number

---

pkg/ccl/changefeedccl/protected_timestamps.go line 74 at r1 (raw file):

		return nil
	})
	addTablePrefix(keys.DescriptorTableID)

should we also add it here?

---

pkg/ccl/changefeedccl/protected_timestamps_test.go line 510 at r1 (raw file):

		row := sqlDB.QueryRow(t, fmt.Sprintf("SELECT range_id FROM [SHOW RANGES FROM TABLE %s.%s]", tableName, databaseName))
		var rangeID int64
		row.Scan(&rangeID)

Could there ever be more than one range?

---

pkg/ccl/changefeedccl/protected_timestamps_test.go line 513 at r1 (raw file):

		refreshPTSReaderCache(s.Clock().Now(), tableName, databaseName)
		t.Logf("enqueuing range %d for mvccGC", rangeID)
		sqlDB.Exec(t, `SELECT crdb_internal.kv_enqueue_replica($1, 'mvccGC', true)`, rangeID)

Wanted to double check my understanding: this would trigger a manual synchronous GC on the range for the tables and we would expect it to not affect the tables that are protected by the PTS?

---

pkg/ccl/changefeedccl/protected_timestamps_test.go line 520 at r1 (raw file):

	sqlDB.Exec(t, "ALTER TABLE foo ADD COLUMN d STRING")
	time.Sleep(2 * time.Second)
	// If you want to GC all sytem tables:

nit: s/sytem/system/

[stevendanna]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andyyang890)

---

pkg/ccl/changefeedccl/protected_timestamps.go line 49 at r1 (raw file):

Previously, andyyang890 (Andy Yang) wrote…

fyi might want to consider updating this number

Done.

---

pkg/ccl/changefeedccl/protected_timestamps.go line 74 at r1 (raw file):

Previously, andyyang890 (Andy Yang) wrote…

should we also add it here?

Done. Nice catch!

---

pkg/ccl/changefeedccl/protected_timestamps_test.go line 510 at r1 (raw file):

Previously, andyyang890 (Andy Yang) wrote…

Could there ever be more than one range?

In production absolutely. In this test, it would be very surprising.

---

pkg/ccl/changefeedccl/protected_timestamps_test.go line 513 at r1 (raw file):

Previously, andyyang890 (Andy Yang) wrote…

Wanted to double check my understanding: this would trigger a manual synchronous GC on the range for the tables and we would expect it to not affect the tables that are protected by the PTS?

Yes, this will run the replica through the MVCC GC queue synchronously. The true as the final argument is for skipShouldQueue which means we will alway run the queue's process callback which is what handles running the GC process.

The GC might affect tables even with the PTS record in place, but we expect that the gc threshold to not advance past the PTS. Thus we expect any read at ts to proceed even after sleeping for a little bit.

[andyyang890]

Thanks!

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @stevendanna)

[stevendanna]

bors r=andyyang890

[craig]

Build succeeded:

• local_roachtest_fips
• examples_orms
• windows_build
• linux_arm64_build
• docker_image_amd64
• unit_tests
• acceptance
• macos_amd64_build
• lint
• check_generated_code
• macos_arm64_build
• linux_amd64_build
• linux_amd64_fips_build
• local_roachtest