ttl: do not remove all table TTL settings on RESET (ttl_expire_after) #110252

ecwall · 2023-09-08T14:28:12Z

Previously running RESET (ttl_expire_after) would remove all table TTL
settings if ttl_expiration_expression was set.

This PR fixes the schema changer to only remove all TTL settings on
RESET (ttl).

Release note (bug fix): RESET (ttl_expire_after) no longer incorrectly
removes ttl_expiration_expression.

cockroach-teamcity · 2023-09-08T14:28:23Z

This change is

pkg/sql/alter_table.go

pkg/sql/schema_changer.go

Xiang-Gu · 2023-09-14T16:00:58Z

pkg/sql/alter_table.go

+		descriptorChanged = true
+	}
+	if !descriptorChanged {
+		// Add TTL mutation so that job is scheduled in SchemaChanger.


super nit: "enqueue TTL mutation ..." might be better since the direction might be add or drop.

Xiang-Gu · 2023-09-14T16:04:37Z

pkg/sql/alter_table.go

+	case before != nil && after == nil:
+		direction = descpb.DescriptorMutation_DROP
+	default:
+		descriptorChanged = true


while you are on this, let's improve around descriptorChanged in the following two ways:

Rename it to something like canDirectlyModifyDescriptor so we know that it's true if we are not enqueuing any mutations and can directly modify the descriptor here;

Return value from this function will be something like canDirectlyModifyDescriptor || hasEnqueuedAnyMutation

Xiang-Gu

Good work! I have two last questions:

If both ttl_expire_after and ttl_expirataion_expression are set, and we reset ttl_expire_after, the new code path, as I understand it, will be
1). Enqueue a DropColumn mutation (so we drop "expire_at" column)
2). Don't enqueue a TTL mutation but instead directly modify the TTL field on the descriptor
Correct?
If we only have ttl_expire_after set, and we reset it, it seems to me the same two things above will happen, but maybe later in descriptor validation, we will reject it, right?

pkg/sql/logictest/testdata/logic_test/row_level_ttl

ecwall · 2023-09-14T18:24:23Z

Good work! I have two last questions:

If both ttl_expire_after and ttl_expirataion_expression are set, and we reset ttl_expire_after, the new code path, as I understand it, will be
1). Enqueue a DropColumn mutation (so we drop "expire_at" column)
2). Don't enqueue a TTL mutation but instead directly modify the TTL field on the descriptor
Correct?

If we only have ttl_expire_after set, and we reset it, it seems to me the same two things above will happen, but maybe later in descriptor validation, we will reject it, right?

1-1) It drops crdb_internal_expiration not expire_at.
1-2) It will not directly modify the descriptor because the mutation will modify the descriptor.
2) You can't reset ttl_expire_after if ttl_expiration_expression is not set - this did not change and there is a subtest for it.

Xiang-Gu · 2023-09-14T18:52:00Z

It drops crdb_internal_expiration not expire_at.

Oh, my previous thoughts were wrong. Thanks!

It will not directly modify the descriptor because the mutation will modify the descriptor.

This is somewhat surprising. Are you sure? In this case, before != nil && after != nil so no TTL mutations will be enqueued and the descriptor is modified right away. Or did I miss something?

This question and two of my unresolved comments are the only things I have. Thanks for doing this!

rafiss

lgtm, apart from Xiang's comments

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @chrisseto, @ecwall, and @Xiang-Gu)

ecwall · 2023-09-14T20:08:08Z

It will not directly modify the descriptor because the mutation will modify the descriptor.

This is somewhat surprising. Are you sure? In this case, before != nil && after != nil so no TTL mutations will be enqueued and the descriptor is modified right away. Or did I miss something?

This code is confusing because it may also be modified in the before != nil && after != nil block, but after that it will either add a mutation or change the descriptor immediately. I did not change this behavior.

ecwall

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @chrisseto and @Xiang-Gu)

pkg/sql/alter_table.go line 1958 at r6 (raw file):

Previously, Xiang-Gu (Xiang Gu) wrote…

while you are on this, let's improve around descriptorChanged in the following two ways:

Rename it to something like canDirectlyModifyDescriptor so we know that it's true if we are not enqueuing any mutations and can directly modify the descriptor here;

Return value from this function will be something like canDirectlyModifyDescriptor || hasEnqueuedAnyMutation

I took this name from descriptorChanged at the call site. If you want to change this, can you file a separate issue?

The call site documents that descriptorChanged should be true only if no mutations are added so it should be changed there also.

// Commands can either change the descriptor directly (for  
// alterations that don't require a backfill) or add a mutation to  
// the list.  
descriptorChanged := false

pkg/sql/alter_table.go line 1961 at r6 (raw file):

Previously, Xiang-Gu (Xiang Gu) wrote…

super nit: "enqueue TTL mutation ..." might be better since the direction might be add or drop.

I took this from the function name I am calling (AddModifyRowLevelTTLMutation) the function calling handleTTLStorageParamChange also refers to it as "add a mutation" so can you file a separate issue to rename it everwhere?

scheduleID and createStmt indexes used to populate slice literals for show results. Rename these to avoid accidental usage and to prevent shadowing in the sql package. Release note: None

Xiang-Gu

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

pkg/sql/alter_table.go line 1958 at r6 (raw file):

Previously, ecwall (Evan Wall) wrote…

I took this name from descriptorChanged at the call site. If you want to change this, can you file a separate issue?

The call site documents that descriptorChanged should be true only if no mutations are added so it should be changed there also.
// Commands can either change the descriptor directly (for  
// alterations that don't require a backfill) or add a mutation to  
// the list.  
descriptorChanged := false

I think descriptorChanged means is there any updates made to the descriptor. We further make updates into two categories:
1). enqueue a mutation to the descriptor's mutation slice
2). modify other fields directly in the descriptor

In handleTTLStorageParamChange, descriptorChanged is not correctly set according to this standard. For example, it's set to false when we enqueue a mutation. That's why I suggested we should rename the variable (within handleTTLStorageParamChange) to something like canDirectlyModifyDescriptor. And that's also why the return value of handleTTLStorageParamChange should be something like canDirectlyModifyDescriptor || hasEnqueuedAnyMutation. Nothing outside handleTTLStorageParamChange should change.

Split large subtests into smaller subtests to make debugging easier. Release note: None

Fixes #109138 Previously running `RESET (ttl_expire_after)` would remove all table TTL settings if `ttl_expiration_expression` was set. This PR fixes the schema changer to only remove all TTL settings on `RESET (ttl)`. Release note (bug fix): `RESET (ttl_expire_after)` no longer incorrectly removes `ttl_expiration_expression`.

ecwall

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @chrisseto and @Xiang-Gu)

pkg/sql/alter_table.go line 1958 at r6 (raw file):

Previously, Xiang-Gu (Xiang Gu) wrote…

I think descriptorChanged means is there any updates made to the descriptor. We further make updates into two categories:
1). enqueue a mutation to the descriptor's mutation slice
2). modify other fields directly in the descriptor

In handleTTLStorageParamChange, descriptorChanged is not correctly set according to this standard. For example, it's set to false when we enqueue a mutation. That's why I suggested we should rename the variable (within handleTTLStorageParamChange) to something like canDirectlyModifyDescriptor. And that's also why the return value of handleTTLStorageParamChange should be something like canDirectlyModifyDescriptor || hasEnqueuedAnyMutation. Nothing outside handleTTLStorageParamChange should change.

Please make another ticket to address this with example(s) of where it is being set to true when a mutation is added. Based on the usage of descriptorChanged, it only needs to be set if no mutations are added:

cockroach/pkg/sql/alter_table.go

Line 849 in 1755d67

if !addedMutations && !descriptorChanged {

Xiang-Gu

I will take a look at the examples. LGTM! Good work!

Xiang-Gu · 2023-09-15T16:29:29Z

@ecwall here you go #110727

ecwall · 2023-09-15T17:04:09Z

bors r=Xiang-Gu

ecwall · 2023-09-15T17:04:31Z

@ecwall here you go #110727

Great, thanks.

craig · 2023-09-15T17:59:16Z

Build succeeded:

Bazel Essential CI (Cockroach)

blathers-crl · 2023-09-15T17:59:30Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

error creating merge commit from 3e7bb4b to blathers/backport-release-22.1-110252: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.1.x failed. See errors above.

error creating merge commit from 5fba729 to blathers/backport-release-23.1-110252: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.x failed. See errors above.

error creating merge commit from 5fba729 to blathers/backport-release-23.1.11-rc-110252: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.11-rc failed. See errors above.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

ecwall requested a review from a team September 8, 2023 14:28

ecwall requested review from a team as code owners September 8, 2023 14:28

ecwall requested a review from a team September 8, 2023 14:28

ecwall requested review from a team as code owners September 8, 2023 14:28

ecwall requested a review from a team September 8, 2023 14:28

ecwall requested a review from a team as a code owner September 8, 2023 14:28

ecwall requested review from abarganier, rachitgsrivastava, DarrylWong, rhu713, sumeerbhola, miretskiy and michae2 and removed request for a team September 8, 2023 14:28

ecwall removed the request for review from a team September 8, 2023 14:29

ecwall requested review from Xiang-Gu and chrisseto September 13, 2023 23:06

ecwall commented Sep 13, 2023

View reviewed changes

pkg/sql/alter_table.go Show resolved Hide resolved

pkg/sql/schema_changer.go Show resolved Hide resolved

pkg/sql/schema_changer.go Show resolved Hide resolved

Xiang-Gu reviewed Sep 14, 2023

View reviewed changes

Xiang-Gu suggested changes Sep 14, 2023

View reviewed changes

Xiang-Gu reviewed Sep 14, 2023

View reviewed changes

pkg/sql/logictest/testdata/logic_test/row_level_ttl Show resolved Hide resolved

ecwall requested a review from Xiang-Gu September 14, 2023 18:24

rafiss reviewed Sep 14, 2023

View reviewed changes

ecwall commented Sep 14, 2023

View reviewed changes

sql: rename scheduleID to scheduleIDIdx and createStmt to createStmtIdx

3e7bb4b

scheduleID and createStmt indexes used to populate slice literals for show results. Rename these to avoid accidental usage and to prevent shadowing in the sql package. Release note: None

Xiang-Gu reviewed Sep 15, 2023

View reviewed changes

ecwall added 2 commits September 15, 2023 11:34

ttl: refactor row_level_ttl logic tests

5fba729

Split large subtests into smaller subtests to make debugging easier. Release note: None

ecwall commented Sep 15, 2023

View reviewed changes

ecwall requested a review from Xiang-Gu September 15, 2023 15:50

Xiang-Gu approved these changes Sep 15, 2023

View reviewed changes

Xiang-Gu mentioned this pull request Sep 15, 2023

ttl: handleTTLStorageParamChange might return incorrect descriptorChanged value #110727

Closed

craig bot merged commit 55731bd into cockroachdb:master Sep 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ttl: do not remove all table TTL settings on RESET (ttl_expire_after) #110252

ttl: do not remove all table TTL settings on RESET (ttl_expire_after) #110252

ecwall commented Sep 8, 2023

cockroach-teamcity commented Sep 8, 2023

Xiang-Gu Sep 14, 2023

Xiang-Gu Sep 14, 2023

Xiang-Gu left a comment

ecwall commented Sep 14, 2023

Xiang-Gu commented Sep 14, 2023

rafiss left a comment

ecwall commented Sep 14, 2023

ecwall left a comment

Xiang-Gu left a comment

ecwall left a comment

Xiang-Gu left a comment

Xiang-Gu commented Sep 15, 2023

ecwall commented Sep 15, 2023

ecwall commented Sep 15, 2023

craig bot commented Sep 15, 2023

blathers-crl bot commented Sep 15, 2023

ttl: do not remove all table TTL settings on RESET (ttl_expire_after) #110252

ttl: do not remove all table TTL settings on RESET (ttl_expire_after) #110252

Conversation

ecwall commented Sep 8, 2023

cockroach-teamcity commented Sep 8, 2023

Xiang-Gu Sep 14, 2023

Choose a reason for hiding this comment

Xiang-Gu Sep 14, 2023

Choose a reason for hiding this comment

Xiang-Gu left a comment

Choose a reason for hiding this comment

ecwall commented Sep 14, 2023

Xiang-Gu commented Sep 14, 2023

rafiss left a comment

Choose a reason for hiding this comment

ecwall commented Sep 14, 2023

ecwall left a comment

Choose a reason for hiding this comment

Xiang-Gu left a comment

Choose a reason for hiding this comment

ecwall left a comment

Choose a reason for hiding this comment

Xiang-Gu left a comment

Choose a reason for hiding this comment

Xiang-Gu commented Sep 15, 2023

ecwall commented Sep 15, 2023

ecwall commented Sep 15, 2023

craig bot commented Sep 15, 2023

blathers-crl bot commented Sep 15, 2023