ttl: refactor storage param Setter to not modify TableDescriptor TTL settings directly

[ecwall]

refs #88254

Changes the storage param Setter to modify UpdatedRowLevelTTL instead of the TableDescriptor directly so that the update can more easily be moved into a schema changer mutation if necessary.

Release note: None

[cockroach-teamcity]

This change is 

[ecwall]

This is redundant with the conditional above.

[ecwall]

Instead of always modifying the TableDesc in Set()/Reset() and possibly unsetting it in handleTTLStorageParams() if there is a mutation. It will only be set in handleTTLStorageParams() if it is not done in a mutation.

[ecwall]

This is the just code refactor from #88260, but does not contain any bug fixes to make reviewing easier.

[rafiss]

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

---

pkg/sql/alter_table.go line 1960 at r1 (raw file):

		telemetry.Inc(sqltelemetry.RowLevelTTLDropped)

		if before.HasDurationExpr() {

i didn't follow why this got removed

---

pkg/sql/alter_table.go line 1983 at r1 (raw file):

	// Validate the type and volatility of ttl_expiration_expression.
	if after != nil && after.HasExpirationExpr() {

or why this got removed

---

pkg/sql/create_table.go line 1337 at r1 (raw file):

	}
	updatedRowLevelTTL := setter.UpdatedRowLevelTTL
	if updatedRowLevelTTL != nil {

nit: you could inline this with if updatedRowLevelTTL := setter.UpdatedRowLevelTTL; updatedRowLevelTTL != nil { ... }

[ecwall]

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

---

pkg/sql/alter_table.go line 1960 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i didn't follow why this got removed

It will always be true because it is already checked inside (before != nil && before.HasDurationExpr()) && (after == nil || !after.HasDurationExpr()).

---

pkg/sql/alter_table.go line 1983 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

or why this got removed

ValidateTTLExpirationExpression() already has this check inside it.

func ValidateTTLExpirationExpression(
	ctx context.Context,
	tableDesc catalog.TableDescriptor,
	semaCtx *tree.SemaContext,
	tableName *tree.TableName,
	ttl *catpb.RowLevelTTL,
) error {

	if !ttl.HasExpirationExpr() {
		return nil
	}
        ...

[ecwall]

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

---

pkg/sql/create_table.go line 1337 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: you could inline this with if updatedRowLevelTTL := setter.UpdatedRowLevelTTL; updatedRowLevelTTL != nil { ... }

Yeah that looks cleaner.

Code quote:

updatedRowLevelTTL := setter.UpdatedRowLevelTTL

[rafiss]

lgtm! good cleanup

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

---

pkg/sql/alter_table.go line 1960 at r1 (raw file):

Previously, ecwall (Evan Wall) wrote…

It will always be true because it is already checked inside (before != nil && before.HasDurationExpr()) && (after == nil || !after.HasDurationExpr()).

ah thanks

---

pkg/sql/alter_table.go line 1983 at r1 (raw file):

Previously, ecwall (Evan Wall) wrote…

ValidateTTLExpirationExpression() already has this check inside it.

func ValidateTTLExpirationExpression(
	ctx context.Context,
	tableDesc catalog.TableDescriptor,
	semaCtx *tree.SemaContext,
	tableName *tree.TableName,
	ttl *catpb.RowLevelTTL,
) error {

	if !ttl.HasExpirationExpr() {
		return nil
	}
        ...

nice

[ecwall]

bors r=rafiss

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)