spanconfigsqlwatcher: add a system.protected_ts_records decoder

[adityamaru]

This change adds a decoder to decode rows from the
system.protected_ts_records table. This will be used by the
rangefeed we setup in the SQLWatcher, to decode rangefeed updates
and subsequently feed them into the SQLTranslator. The target column
of the system table is the only column of interest since it will
determine whether we ask the translator to generate SystemSpanConfigs
or regular SpanConfigs. More information about this will be added
in follow up PRs that use this decoder.

This change also instructs gogoproto to generate an equality method
for roachpb.TenantID, to allow for equality checks in the decoder
test.

Informs: #73727

Release note: None

[cockroach-teamcity]

This change is 

[adityamaru]

@dt adding you for the small change to roachpb.TenantID

[dt]

for the small change to roachpb.TenantID

I'll defer to @tbg or @RaduBerinde if we want the proto message to be comparable (I'd guess yes -- the go struct was already comparable -- so I don't see why not, but we treat TenantID with care so let's check)

[RaduBerinde]

Same here, I don't see why not.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @dt)

[tbg]

That seems ok.

[irfansharif]

Instead of writing to the real table in this unit test, it'd be better isolated if using a scratch table instead:

cockroach/pkg/spanconfig/spanconfigkvsubscriber/span_config_decoder_test.go

Lines 40 to 42 in 89f8aba

	const dummyTableName = "dummy_span_configurations"
	tdb := sqlutils.MakeSQLRunner(tc.ServerConn(0))
	tdb.Exec(t, fmt.Sprintf("CREATE TABLE %s (LIKE system.span_configurations INCLUDING ALL)", dummyTableName))

Given this test should only care about the decoding routines, I would suggest writing to the scratch table in this way directly, instead of depending on jobs or the protected ts subsystem to write to the real table (and also needing to release the record). Also, since the column is nullable, worth having a nil check (an empty proto should be returned -- something we'll need to explicitly handle).

[adityamaru]

Hmm doesn't it make sense to test this with the jobsprotectedts.MakeRecord, Protect, and Release methods that are actually going to be used to interact with this table by all PTS consumers? We use this pattern in many other PTS tests as well. In theory, target is a free-form bytes column and the decoder+test could be doing something different from what the actual system writes to the column, so it seems a little scary to decouple it from how a pts record will actually be written?

[irfansharif]

Hm, I'm not sure, but I see your point. I was coming at it to minimize the test/make it fast enough to also then suggest writing a benchmark for it ("TestProtectedTimestampDecoder only cares about decoding"). Minimizing the test also prevents it from failing when another subsystem is buggy (Protect/Release); the nice thing about talking through this table is that these sub-systems are decoupled, I think it's fine/sane to have them decoupled in test code as well. I'll defer to you and not hold up the PR.

[adityamaru]

Minimizing the test also prevents it from failing when another subsystem is buggy

Coming from bulkio land where we write almost all tests that rely on the jobs subsystem, this is something I've just taken for granted before heh. The benchmarking is a good point, I'm leaning towards merging this for now since I have CI happy, and cleaning it up during stability if we decide to benchmark some of these pieces.

[adityamaru]

TFTR!

bors r=irfansharif

[craig]

Build succeeded:

• GitHub CI (Cockroach)