Change looks good as is, but IMO an alternative worth considering would be to keep the interface as is and teach the KVSubscriber's implementation to ignore certain protection policies when querying it for protected timestamps. The KVSubscriber already has the SpanConfig and this would ensure the caller doesn't have to join the two fields to filter out certain protection policies.

Reviewed 12 of 13 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @arulajmani)

pkg/kv/kvserver/protectedts/ptcache/cache.go, line 139 at r1 (raw file):

		func(rec *ptpb.Record) (wantMore bool) {
			protectionPolicies = append(protectionPolicies,
				roachpb.ProtectionPolicy{ProtectedTimestamp: rec.Timestamp})

Could you write a comment here explaining why the default value of IgnoreIfExcludedFromBackup is the right thing?

pkg/spanconfig/spanconfig.go, line 410 at r1 (raw file):

// protectedts.Cache.
type ProtectedTSReader interface {
	// GetProtectionPolicies returns all protected timestamps that apply to any

nit: s/all protected timestamps/all protection policies/g

pkg/kv/kvserver/replica_protected_timestamp_test.go, line 63 at r1 (raw file):

				mp.protections = append(mp.protections, manualPTSReaderProtection{
					sp:                 roachpb.Span{Key: keys.MinKey, EndKey: keys.MaxKey},
					protectionPolicies: []roachpb.ProtectionPolicy{{ProtectedTimestamp: ts}},

nit: for all these, you could define yourself a helper that constructs protection policies given a list of timestamps.

pkg/kv/kvserver/protectedts/ptcache/cache_test.go, line 412 at r1 (raw file):

						protections[j].ProtectedTimestamp)
				})
				require.Equal(t, []roachpb.ProtectionPolicy{

nit: Similar to my comment above, might be worth extracting this out in a helper that can give you a list of protection policies given a list of timestamps.

pkg/spanconfig/spanconfigptsreader/adapter_test.go, line 68 at r1 (raw file):

	// Add some records to the cache; ensure they're returned.
	mc.protectionPolicies = append(mc.protectionPolicies,

nit: here as well, if you decide to take my suggestion.

Thanks for making the change, this is much cleaner!

Reviewed 14 of 16 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @arulajmani)

pkg/spanconfig/spanconfigkvsubscriber/kvsubscriber_test.go, line 33 at r2 (raw file):

}

func tableSpan(tableID uint32) roachpb.Span {

nit: can we make this a local function to the test?

pkg/spanconfig/spanconfigkvsubscriber/kvsubscriber_test.go, line 65 at r2 (raw file):

	sp43Cfg.cfg.ExcludeDataFromBackup = true

	subscriber := &KVSubscriber{}

Can we create a test cluster to get all dependencies and replace this with a call to spanconfigkvsubscriber.New instead?

pkg/spanconfig/spanconfigkvsubscriber/kvsubscriber_test.go, line 122 at r2 (raw file):

}

func makeSpanAndSpanConfigWithProtectionPolicies(

nit: can we make this a local function to the test?

pkg/spanconfig/spanconfigkvsubscriber/kvsubscriber_test.go, line 143 at r2 (raw file):

	context.Context, bool, ...spanconfig.Update,
) (deleted []spanconfig.Target, added []spanconfig.Record) {
	return nil, nil

Here, and below, let's make these panic?

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

pkg/spanconfig/spanconfigkvsubscriber/kvsubscriber_test.go, line 65 at r2 (raw file):

just called into spanconfigkvsubscriber.New with everything nil'ed out and that works too.

pkg/spanconfig/spanconfigkvsubscriber/kvsubscriber_test.go, line 143 at r2 (raw file):

Done.