Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @arulajmani, @dt, @irfansharif, @nvanbenschoten, and @stevendanna)

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 73 at r2 (raw file):

lay protected timestamp on (say) databases and have them take effect on all tables inside the database 
(including those created in the future!). This is quite powerful as it allows us to 
[decouple scheduled backups with revision history from the GC TTL] (https://github.com/cockroachdb/cockroach/issues/67282) 

nit: extra space breaks the link

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 112 at r2 (raw file):

The new PTS subsystem will operate on schema objects instead of a list of static spans. Schema 
objects form the catalog hierarchy (“cluster”->database->schema->table->index->partition). Leaf 

(“cluster”->database->schema->table->index->partition->subpartition->subpartition->...) technically

Code quote:

“cluster”->database->schema->table->index->partition

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 134 at r2 (raw file):

The new invariant is slightly unsatisfying for users of the subsystem because there can be an 
arbitrary amount of time between writing the PTS record and span configuration reconciliation. 

I think it's worth forwarding a link to a later section where we talk about the possibility of the client waiting for reconciliation.

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 618 at r2 (raw file):

## Future Work

I think here or in an Open Questions section we should discuss clients being able to observe reconciliation timestamps and what a client-observable invariant might look like.

Reviewed all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @arulajmani, @dt, @irfansharif, and @stevendanna)

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 5 at r2 (raw file):

- Start Date: 2021-11-11
- Authors: Arul Ajmani, Aditya Maru
- RFC PR: (PR # after acceptance of initial draft)

#74685

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 14 at r2 (raw file):

GC runs on the host tenant

I don't think this is quite how we'd describe things today. GC runs in KV, not on a particular tenant. However, KV only checks the host tenant's PTS table today.

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 25 at r2 (raw file):

The rework of the PTS subsystem proposed here builds on top of the newly introduced 
[span configuration infrastructure](https://github.com/cockroachdb/cockroach/blob/master/docs/RFCS/20210610_tenant_zone_configs.md) 
as the transport layer. The RFC proposes how the existing span configuration infrastructure can be 

"transport layer" between what and what?

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 184 at r2 (raw file):

```protobuf
message SystemSpanConfig {

Did we explore the alternative of using the same SpanConfig type here? Why use a special type with fewer fields? SpanConfig inheritance is across all existing fields is well-defined.

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 287 at r2 (raw file):

Internally, we will continue to store the PTS records in the `system.protected_ts_records` table. 
We will add a bytes `target` column alongside the existing `spans` column, to store the `ptpb.Target`
protobuf which will describe what object the record is protecting.

Let's mention here that the system.protected_ts_records table is indexed on this UUID, so addressing a PTS record by its id is a point operation.

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 345 at r2 (raw file):

  // GetProtectedTimestampsForCluster returns all the protected timestamps that
  // apply to the entire cluster's keyspace.
  GetProtectedTimestampsForCluster() []hlc.Timestamp

Should these return raw hlc.Timestamps, or a message wrapping the timestamp and the ProtectionMode? I guess this question also applies to the new field in SystemSpanConfig. cc. @ajwerner

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 394 at r2 (raw file):

Full reconciliation does not change much with the introduction of `SystemSpanConfigs` -- much of the
heavy lifting will be continue to be done by the `FullTranslate` method of the `SQLTranslator`. In 

s/will be continue/will continue/

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 405 at r2 (raw file):

transaction in the `SQLTranslator` that was used to intialize the `SystemPTSRecordTableReader` we 
will ensure that the protected timestamp state update to KV is represents a consistent view from the
tenant's perspective.

Is this paragraph assuming a single reconciliation batch and no pagination across the KV connector?

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 652 at r2 (raw file):

unchanged which inturn simplifies the migration from the old subsystem to the new one.

### Building a fully generalized hierrarchy for span configurations

s/hierrarchy/hierarchy/

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 684 at r2 (raw file):

of change to the newly introduced span configuration infrastructure which is yet to be hardened. 
The fully generalized scheme is also more involved and would require a complex batching scheme when 
reconciling the entire SQL state with KV. 

Do you mind adding one more sentence to this about what was complex about this batching scheme?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @arulajmani, @dt, @irfansharif, @nvanbenschoten, and @stevendanna)

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 189 at r2 (raw file):

  // ProtectedTimestamps is a list of timestamps which are protected from being
  // GC-ed.
  repeated util.hlc.Timestamp protected_timestamps = 1  [(gogoproto.nullable) = false];

Wrap this in a message type for future extensions?

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 345 at r2 (raw file):

Even if we don't add a ProtectionMode field at this point (the whole yagni thing and the winds seem to be blowing towards nobody really wanting ProtectAt). I feel fine about this for a go interface for now, we can always add more wrapping later. However, in protos, we should take care to wrap the timestamps in a message so we can add more info there. That's my 2c on this at this point.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @ajwerner, @arulajmani, @dt, @irfansharif, @nvanbenschoten, and @stevendanna)

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 184 at r2 (raw file):

We considered using the same SpanConfig proto but decided to introduce this special type once the design moved away from generalized inheritance. As the hierarchy we're proposing is strictly above RANGE DEFAULT, SpanConfigs are guaranteed to be fully hydrated so we can never have inheritance of fields. We made this fully hydrated distinction explicit by choosing to make all fields on SpanConfigs non-nullable (unlike zone configs) and introducing this new message lets us keep things that way.

Now that I type this, maybe this fully hydrated argument may not apply in the future if we introduce new fields on SpanConfigs to be only set by the host tenant and apply to all tenant ranges. But a different type here shouldn't preclude us from doing so in the future.

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 194 at r2 (raw file):

// SystemSpanConfigEntry is a SystemSpan and its corresponding SystemSpanConfig.
message SystemSpanConfigEntry {
  Span systemSpan = 1 [(gogoproto.nullable) = false];

Wanted to call out that this might change as described in #74765 (comment).

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 345 at r2 (raw file):

That's a fair point -- I didn't have it initially because we weren't making use of anything but the timestamp in KV, but I like the idea of future proofing by wrapping this in a message on the proto but keeping the interface as is for now.

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 405 at r2 (raw file):

Yeah, or, at the least, it assumes that even if we paginate across the connector we will only write the updates in a single transaction. I'm not sure if doing so is reasonable.

It's also worth capturing the discussion we had in the zone configs pod ( @ajwerner , @irfansharif , @nvanbenschoten , @adityamaru , and I) about how much we care about this. While it does make things easier to reason about and I think it's something we should try to do unless prohibitive, Andrew brought up a good point that it isn't really a guarantee consumers of the subsystem would really care about -- instead, consumers of the subsystem
are interested in when their span config (with the protection on it) is reconciled and has made its way down to KV.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @ajwerner, @arulajmani, @dt, @irfansharif, @nvanbenschoten, and @stevendanna)

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 6 at r2 (raw file):

- Authors: Arul Ajmani, Aditya Maru
- RFC PR: (PR # after acceptance of initial draft)
- Cockroach Issue: #73727

nit: it's customary to write [#NNNN](http://link/to/issue/NNNN) so that we get the hyperlink when viewing the rendered doc.

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 684 at r2 (raw file):

of change to the newly introduced span configuration infrastructure which is yet to be hardened. 
The fully generalized scheme is also more involved and would require a complex batching scheme when 
reconciling the entire SQL state with KV. 

Maybe also worth a note about the complexity of detecting cycles to protect against low-level mistakes / bugs.

This RFC for me feels 90% done; most of it feels sound. While there are some specific impl choices around API signatures and typing, they feel relatively minor to me and I'd be happy bottoming out on them wherever you prefer (here, or in actual PRs introducing them). A few of my comments below are marked with "NB:" and are worth agreeing upon (maybe in our pod today?), everything else is a nit or questions for my own understanding.

I see we've been tracking individual PRs/work-breakdown as part of #73727
(a few have already landed!). I encourage you to fill in more details there, and maybe mark exactly what's needed for 22.1. I think we know now with this RFC, but it's a good place to coordinate and communicate from.

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru, @ajwerner, @arulajmani, @dt, @irfansharif, @nvanbenschoten, and @stevendanna)

-- commits, line 15 at r2:
[nit] Same as Nathan's comment below, more words to explain what we mean by "transport" (something something shipping of tenant scoped/keyspace protection records down into KV, where admittance is mediated by KV -- but worded better).

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 14 at r2 (raw file):

+1.

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 24 at r2 (raw file):

The rework of the PTS subsystem proposed here builds on top of the newly introduced 
[span configuration infrastructure](https://github.com/cockroachdb/cockroach/blob/master/docs/RFCS/20210610_tenant_zone_configs.md) 

[nit] All throughout the doc: refer to the other RFCs/packages/files by SHA/permalink instead of master, path might break in the future.

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 70 at r2 (raw file):

In addition to building feature parity, the new protected timestamp subsystem aims to express PTS 
records in terms of schema objects instead of static keyspans.This allows users of the subsystem to 

[nit] Missing space: "keyspans.This"

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 78 at r2 (raw file):

wide default GC TTL set to 25 hours.

The introduction of hierarchy in KV for span configurations is necessitated by the desire for the host 

[nit] Maybe caveat this by the "limited" or "static" form that this RFC is proposing below?

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 81 at r2 (raw file):

tenant to lay protected timestamps on one or all secondary tenants’ keyspace. This is required when 
the host tenant performs full tenant or full cluster backups, respectively. We also want to allow a 
“fast path” for secondary tenants to set protected timestamps on their entire keyspace with no write

[nit] s/no write/minimal write

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 101 at r2 (raw file):

the `Connector`. KV then uses its view of the reconciled state to make various split, merge, and 
rebalancing decisions. In line with this SQL/KV separation, the design below includes two orthogonal
proposals, each with its own set of alternatives, to make protected timestamps work with multi-tenancy:

This way of structuring the RFC is pretty clarifying and easy to grok, thanks for doing it.

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 122 at r2 (raw file):

Any future leaves in the subtree will also be protected until the record is removed.

The original RFC provided an invariant in terms of the transaction that wrote the PTS record. The 

[nit] Link to the relevant section in the PTS RFC.

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 133 at r2 (raw file):

The new invariant is slightly unsatisfying for users of the subsystem because there can be an

Is it any more/less "unsatisfying" than before? There was an arbitrary delay earlier with gossip propagation of the GC TTL before too, right? And the internal polling of protectedts.Cache? The only invariant change AFAIU is that we're talking about the txn writing to system.span_configurations vs. the txn writing to system.zones, which does not feel very controversial or that noticeably different to readers. The "invariant changes", if any, are related to us omitting protectedts verification because of non-destructive retry behavior -- something we've explained clearly below. If you agree, I'd maybe de-emphasize these few paragraphs.

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 141 at r2 (raw file):

Lastly, it's worth calling out that even though KVs view of protected timestamp state for a tenant 
may be arbitrarily stale, it *will* be a consistent view that was valid at some timestamp in the 

Possibly subject to change; see top-level comment.

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 177 at r2 (raw file):

`SystemSpans` and their associated `SystemSpanConfigs`  will be stored in the 
`system.span_configurations` table. `SystemSpanConfigs` will only include protected timestamp 

Worth noting a (discarded) alternative here/below: Using a separate table for these special keys. Also worth nothing that these keys are only an implementation detail within the KVAccessor; nothing else would/should know about them.

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 189 at r2 (raw file):

Maybe even folding it into the GC message, since they're so closely coupled.

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 194 at r2 (raw file):

NB: Re-using the Target definition below in the "PTS Record" section seems like the most obvious thing to do, and I think is close what you're already doing in https://github.com/cockroachdb/cockroach/pull/74765/files.

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 266 at r2 (raw file):

Users of the protected timestamp subsystem will continue to interact with the subsystem using the 
`Storage` interface. It will minimally change to the following:

[nit] When spelling out interface/proto changes, I feel these would be read much clearer if presented as "diff"s. They should be easy enough to create. Given the contents of the RFC assume some degree of familiarity with two others, I think "diff"s in this context would help rather than hurt.

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 287 at r2 (raw file):

Lets also mention (either here or below) that few components (SQLTranslator) will perform full table scans over this table, but we don't think that's a problem given there should be O(concurrent backup) number of records, so probably single digits.

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 331 at r2 (raw file):

#### SQLTranslator

The `SQLTranslator` will continue to be responsible for translating SQL state to Span Configurations.

[nit] Lowercase "Span Configurations"

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 356 at r2 (raw file):

As the SystemPTSRecordTableReader is transaction scoped and we want to use the same transaction to

NB: Regurgitating a discussion from elsewhere: this interface/package/type can sit squarely within the SQLTranslator, obviating the need to make it txn scoped or even add to the public API of the span config package. This thinking would refresh a few paragraphs below.

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 414 at r2 (raw file):

#### KVAccessor

We will modify the existing RPCs on the `KVAccessor` to account for `SystemSpanConfigs` as follows:

[nit] Same as above, presenting it as a diff over what's currently there would draw attention to just the bits that are changing (I realize we've already crafted snippets -- GetSpanConfigsRequest isn't changing, right?)

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 460 at r2 (raw file):

We choose to modify the existing RPCs instead of adding new ones to specifically deal with 
`SystemSpanConfigs` as doing so allows us to read/write both `SystemSpanConfigs` and `SpanConfigs` 
in the same transaction. This allows us to ensure that KV always sees a consistent view of the 

NB: Regurgitating a discussion from elsewhere: It's hard to ensure a consistent view in the presence of hierarchy + pagination. Thankfully, for protected timestamps, that's not necessary. I don't know that this necessary changes whether we should use a single RPC with four arguments or two RPCs with two each, I don't have a strong feeling either way. I would lean towards whatever signature ends up looking cleaner after we've settled on the appropriate types, not something we need to bottom out in this RFC itself.

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 468 at r2 (raw file):

The `SystemSpanConfigStore` will be an in-memory representation of all `SystemSpanConfig`s and will 
be populated by the `KVSubscriber`. It will be consulted by the `KVSubscriber` to synthesize 
protected timestamp information when queried for a span's `SpanConfig`.

Maybe a few words about the concrete impl. AFAIU, it's very simple, a glorified map effectively, right?

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 481 at r2 (raw file):

deleted []roachpb.Span

NB: Relates to the point about message typing for SystemSpanConfigEntry. By using a Span underneath, we're leaking internal details. This could very well be deleted []roachpb.SystemSpanConfigEntry. TBD on the need to use a dedicated SystemSpanConfigEntry type vs. re-using an existing one -- another thing I don't have strong opinions/preferences for. There too I would just lean on whatever ends up looking better in code, not necessarily worth bottoming out on in this RFC.

[mega nit] Can't think of a better name for SystemSpanConfigEntry, but we have several variants now which if anything is at least funny: SystemConfigSpan, SpanConfig, SystemSpanConfig, system.span_configurations. At least one of them will go away soon.

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 523 at r2 (raw file):

timestamp information from the `KVSubscriber`. The `ProtectedTSReader` interface will also be useful
for the migration of the subsystem as we will store protectedTS information in both 
`system.protected_ts_records` and `SpanConfigurations` in 22.1 (more details in the migration 

[nit] s/SpanConfigurations/sytem.span_configurations

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 536 at r2 (raw file):

  // the given keyspan and returns the least timestamp that is above the supplied  
  // threshold.
  GetEarliestProtectionTimestampAboveThreshold(ctx context.Context, sp roachpb.Span,    

For my own understanding, why does the threshold factor in? Why not just return the most conservative protection timestamp for any part of the given span? And let the GC queue itself care about threshold logic.

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 553 at r2 (raw file):

Once the above migration runs, we can stop persisting the `spans` of any new `ptpb.Record` in the 
`system.protected_ts_records` table. This way all calls to `Protect` after the migration has run 

Do we need a "ensure no old records exist" cluster version in the following release? We could also be aggressive and do it now in the 22.1 cycle, but I don't know if it buys us much. Is there a reason not to do it now however?

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 573 at r2 (raw file):

on spans but on schema objects. In addition to this we will want to enforce limits when reconciling 
through the `KVAccessor`. The exact semantics of limit checking are TBD but much of the work here 
will tack on to [what we do for span configurations](https://github.com/cockroachdb/cockroach/issues/70555).

I don't think the limits we're linking to here is the same as what this section is describing, or what the sister section from the protectedts RFC originally described (where it's more about the size of the PTS records themselves). I'd either omit this link, or drop this section entirely.

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 586 at r2 (raw file):

[here](https://github.com/cockroachdb/cockroach/pull/61004).

In this RFC we propose to not provide verification semantics given the complexity that comes with 

I agree we don't need it, but maybe worth adding a sentence for what this would look like. Would it not just be an RPC out to KV through the Connector that returns a boolean response?

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 646 at r2 (raw file):

it provides us with an implicit `schema object ID -> PTS` record mapping. A PTS record on the entire
cluster (during cluster backups) could be written to `RANGE DEFAULT`, and everything would work out 
of the box! The main deterrent to this approach was that it went against the nature of fields stored

Another deterrent was not having a good home for the record that the host tenant can install to protect all tenants, or a specific tenant. The former could be a RANGE DEFAULT thing, but it's shaky. I don't know what the latter would be.

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 680 at r2 (raw file):

partitions/indexes) under the database; with a fully generalized scheme this would be a single write.

The fully generalized solution was explored in detail during an early iteration of this RFC, but it

Worth noting that for full cluster backups from both the host or individual tenants, and for host initiated backups for specific tenants, the write amp is the same in both proposals. It's only for databases with a large number of tables where they diverge. The divergence too isn't in storage amplification (still O(tables)) but in write amp, which the protected ts field is unique in that being driven by an automatic system, it's frequently changing -- an access pattern that does not hold for other things in zone configs.

Woops, wrong button.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru, @ajwerner, @arulajmani, @dt, @irfansharif, @nvanbenschoten, and @stevendanna)

I feel like there's a bunch of good outstanding comments to be addressed. Can we get one more pass iteration before stamping this and merging it in-progress?

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru, @ajwerner, @arulajmani, @dt, @irfansharif, @nvanbenschoten, and @stevendanna)

docs/RFCS/20211111_multitenant_protected_timestamps.md, line 2 at r2 (raw file):

- Feature Name: Multi-tenant Protected Timestamps Support
- Status: draft

I'd update this to in-progress

I feel like there's a bunch of good outstanding comments to be addressed. Can we get one more pass iteration before stamping this and merging it in-progress?

+1, I don't see any remaining discussion points that should block this from merging, so let's get one more iteration and then it should be good to merge.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru, @ajwerner, @arulajmani, @dt, @irfansharif, @nvanbenschoten, and @stevendanna)