sql: persist TTL metadata to the descriptor

[otan]

These commits allow the WITH option in a table to symbolize TTL.
Furthermore, these values can be introspected.

Refs: #75428

See individual commits for details.

[cockroach-teamcity]

This change is 

[otan]

there is some discussion around #75189 which may change how this initially looks, but i'd rather merge this for now and come back to it as i have a bunch of commits lined up assuming it's what it is right now.

it'll be easier to modify all at once at a later stage.

[postamar]

This is, in and of itself, a change I'm OK with, but it's a bit puzzling to see this syntax added, interface methods defined, etc. without any meaningful usage of these. As a result I feel it cheapens my review somewhat.

Anyway, this is all quite uncontroversial stuff and the motivation is well explained in the RFC, so I'm not against it, but I'm left wondering what's the rush in merging this before pushing more commits to this feature branch with code that actually uses the code you introduce here? Why would you rather merge this for now, as you say? I don't feel unreasonable in asking.

Reviewed 1 of 1 files at r1, 3 of 3 files at r2, 10 of 10 files at r3, 4 of 4 files at r4, 2 of 2 files at r5, 4 of 4 files at r6, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @otan and @rafiss)

---

pkg/sql/catalog/descriptor.go, line 637 at r4 (raw file):

	GetRegionalByRowTableRegionColumnName() (tree.Name, error)
	// GetRowLevelTTL returns the row-level TTL config for the table.
	GetRowLevelTTL() *descpb.TableDescriptor_RowLevelTTL

Would it make sense to replace this with a bunch of methods like

HasRowLevelTTL() bool
RowLevelTTLExpiresAfter() string
...

and so forth? Or define a catalog.RowLevelTTL interface which wraps the descpb proto if that makes more sense?

It's a bit more boilerplate on the interface implementation side but experience has taught us the value of decoupling the descriptor interfaces a bit from the underlying protobufs. From the interface clients' perspective, it shouldn't change things much.

That being said I'm fully aware that we return a protobuf for privileges and we're cool with that. Just something to think about.

---

pkg/sql/paramparse/paramobserver.go, line 103 at r3 (raw file):

// NewTableStorageParamObserver returns a new TableStorageParamObserver.
func NewTableStorageParamObserver(tableDesc *tabledesc.Mutable) *TableStorageParamObserver {
	return &TableStorageParamObserver{tableDesc: tableDesc}

TIL about the StorageParamObserver interface. Neat!

[adityamaru]

with code that actually uses the code you introduce here?

Rather selfishly I want to plug for merging the basic support for storage params to ALTER TABLE and CREATE TABLE, since bulkio is hoping to piggyback on these for implementing #73536. I have a commit that builds on top of some of this infrastructure over in #75295, which in turn unblocks #75451.

[otan]

This is, in and of itself, a change I'm OK with, but it's a bit puzzling to see this syntax added, interface methods defined, etc. without any meaningful usage of these. As a result I feel it cheapens my review somewhat.

I understand - the flip side is I rather incrementally add progress than give you a 2000 LoC PR that uses everything that was defined in a monster 20 commit marathon. Psychologically draining to review that and update the PR, and more likely to introduce one blocking thing that moves progress altogether + prevents branching from the PR to do more.

Why would you rather merge this for now, as you say?

Generally I'd rather merge small increments than big branches at once, where merge skew and changes just adds on and drags over time. With feature gated features (and even better, half yearly release cycles!), it doesn't "harm" anyone to have it in, and if it's a concern reverting is cheaper than rebasing.

[otan]

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

---

pkg/sql/catalog/descriptor.go, line 637 at r4 (raw file):

Previously, postamar (Marius Posta) wrote…

Would it make sense to replace this with a bunch of methods like

HasRowLevelTTL() bool
RowLevelTTLExpiresAfter() string
...

and so forth? Or define a catalog.RowLevelTTL interface which wraps the descpb proto if that makes more sense?

It's a bit more boilerplate on the interface implementation side but experience has taught us the value of decoupling the descriptor interfaces a bit from the underlying protobufs. From the interface clients' perspective, it shouldn't change things much.

That being said I'm fully aware that we return a protobuf for privileges and we're cool with that. Just something to think about.

I can see it providing values for the descriptors atm, not sure I (yet) buy it for the TTL struct.
I'll keep it in mind, if you feel strongly I can submit a follow up to patch it up.

[otan]

thanks! i'm going to merge this to unblock a couple of things :)

bors r=postamar

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[postamar]

Sorry for the belated reply: please do go ahead. Also, I appreciate wanting to keep PRs small and short-lived too.