*: lower default GC TTL to 4h by irfansharif · Pull Request #93836 · cockroachdb/cockroach

irfansharif · 2022-12-16T23:39:13Z

Release note (general change): The GC TTL previously defaulted to 25h. This value was configurable using ALTER RANGE DEFAULT CONFIGURE ZONE USING gc.ttlseconds = <whatever>, but also possible to scope to specific schema objects using ALTER {DATABASE,TABLE,INDEX} CONFIGURE ZONE USING .... This value determine how long overwritten values were retained. The RANGE DEFAULT value is now lowered to 4h but only for freshly created clusters. When existing clusters upgrade onto this release, we will respect whatever value they were using before the upgrade for all their schema objects. This will be 25h if the GC TTL was never altered, or some specific value if set explicitly. Full cluster backups taken on earlier version clusters, when restored to clusters that started off at v23.1, will use the GC TTL recorded in the backup image.

We've found the 25h value to translate to higher-than-necessary storage costs, especially for workloads where rows are deleted frequently. It can also make for costlier reads with respect to CPU since we currently have to scan over overwritten values to get to the one of interest. Finally, we've also observed cluster instability due to large unsplittable ranges that have accumulated an excessive amount of MVCC garbage. We chose a default of 25h originally to accommodate daily incremental backups with revision history. But with the introduction of scheduled backups introduced in 22.2, we no longer need a large GC TTL. Scheduled backups "chain together" and prevent garbage collection of relevant data to ensure coverage of revision history across backups, decoupling it from whatever value is used for GC TTL. So we no longer need a 25h default, hence this change.

The GC TTL determines how far back AS OF SYSTEM TIME queries can go, which now if going past now()-4h, will start failing informatively. To support larger windows for AS OF SYSTEM TIME queries, users are encouraged to pick a more appropriate GC TTL and set it using ALTER ... CONFIGURE ZONE using gc.ttlseconds = <whatever>. The earlier considerations around storage use, read costs, and stability still apply.

Release note (backward-incompatible change): See release note above. Technically this is not a backwards-incompatible change since we're only changing the default value that new clusters are initialized with -- existing clusters will remain unaffected. But it might be worth highlighting this change more prominently to our users for added scrutiny.

cockroach-teamcity · 2022-12-16T23:39:21Z

This change is

knz · 2022-12-17T11:30:28Z

This change in defaults determines how far back AS OF SYSTEM TIME queries can go, which if going past now()-4h, will start failing informatively.

Maybe add another release note "backward-incompatible change", so it gets more scrutiny by users.

irfansharif

Updated all the testlogic files but I'm not sure why there's so much indentation diff, I've used just vanilla dev test{logic} ... --rewrite to power through. It's making for a larger diff than is real -- all that's changing is s/90000/14400 wherever we've rendered the zone config.

Maybe add another release note "backward-incompatible change", so it gets more scrutiny by users.

Done (left a note for the docs team to highlight this more prominently, even though it's not technically backwards compatible). PTAL.

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

irfansharif · 2022-12-19T17:16:08Z

It's making for a larger diff than is real -- all that's changing is s/90000/14400 wherever we've rendered the zone config.

Using git show HEAD --ignore-all-space helps highlight exactly what's going on.

irfansharif · 2023-01-03T17:49:58Z

(Bump.)

postamar · 2023-01-03T21:02:13Z

What's the story for restoring backups here? I only skimmed through the changes but I don't see any injection of zone configs upon restoring pre-23.1 backups, so it's safe to assume that cluster- and partial restores will be subject to this new 4h default as well. Are we cool with that? To me it feels strange.

irfansharif · 2023-01-04T19:05:50Z

Good point, I didn't do any such thing. @dt or @adityamaru, can you maybe show me where I could inject such a hook? We backup+restore system.zones:

cockroach/pkg/ccl/backupccl/system_schema.go

Lines 375 to 376 in 411c8c2

    
           systemschema.ZonesTable.GetName(): { 
        
           	shouldIncludeInClusterBackup: optInToClusterBackup, // ID in "id".

So some point after the system table is restored, and maybe before MVCC GC has a chance to kick in (or before user tables are restored), I go explicitly set the existing GC TTL to whatever it was before? Looking at the code, we do have descriptors that we "pre-"restore before restoring user tables, so somewhere here?

cockroach/pkg/ccl/backupccl/restore_job.go

Lines 1301 to 1302 in abc836c

    
           dataToPreRestore = &restorationDataBase{ 
        
           	spans:        preRestoreSpans,

adityamaru · 2023-01-05T05:54:15Z

cluster- and partial restores will be subject to this new 4h default as well.

Cluster restore deletes the zone configs of the restoring cluster and applies the zone configs of the backed-up cluster. It does this prior to ingesting user data to allow the allocator to distribute the replicas. Non-cluster restores don't deal in zone configs at all, and the ingested data is subject to whatever configuration applies to the table being restored.

The status quo seems okay to me? A cluster restore is the only form of restore that is in the business of altering system tables, so it is what we encourage users to use if they want an exact replica of their backed up cluster.

postamar · 2023-01-05T14:45:55Z

Thanks! All good then.

irfansharif

🤦 Somehow forgot that v22.2 and earlier clusters do actually have an explicit entry in system.zones with the default GC TTL. So didn't need any explicit upgrade/migration. Same reason that we didn't need anything for restores. Updated PR + rebased, PTAL.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @ajwerner, @dt, @herkolategan, and @renatolabs)

adityamaru

Thanks for doing this, it makes me very happy to see us dropping this value.

pkg/sql/logictest/testdata/logic_test/zone_config_system_tenant

Fixes cockroachdb#89233. Release note (general change): The GC TTL previously defaulted to 25h. This value was configurable using `ALTER RANGE DEFAULT CONFIGURE ZONE USING gc.ttlseconds = <whatever>`, but also possible to scope to specific schema objects using `ALTER {DATABASE,TABLE,INDEX} CONFIGURE ZONE USING ...`. This value determine how long overwritten values were retained. The `RANGE DEFAULT` value is now lowered to 4h but only for freshly created clusters. When existing clusters upgrade onto this release, we will respect whatever value they were using before the upgrade for all their schema objects. This will be 25h if the GC TTL was never altered, or some specific value if set explicitly. Full cluster backups taken on earlier version clusters, when restored to clusters that started off at v23.1, will use the GC TTL recorded in the backup image. We've found the 25h value to translate to higher-than-necessary storage costs, especially for workloads where rows are deleted frequently. It can also make for costlier reads with respect to CPU since we currently have to scan over overwritten values to get to the one of interest. Finally, we've also observed cluster instability due to large unsplittable ranges that have accumulated an excessive amount of MVCC garbage. We chose a default of 25h originally to accommodate daily incremental backups with revision history. But with the introduction of scheduled backups introduced in 22.2, we no longer need a large GC TTL. Scheduled backups "chain together" and prevent garbage collection of relevant data to ensure coverage of revision history across backups, decoupling it from whatever value is used for GC TTL. So we no longer need a 25h default, hence this change. The GC TTL determines how far back AS OF SYSTEM TIME queries can go, which now if going past `now()-4h`, will start failing informatively. To support larger windows for AS OF SYSTEM TIME queries, users are encouraged to pick a more appropriate GC TTL and set it using `ALTER ... CONFIGURE ZONE using gc.ttlseconds = <whatever>`. The earlier considerations around storage use, read costs, and stability still apply. Release note (backward-incompatible change): See release note above. Technically this is not a backwards-incompatible change since we're only changing the default value that new clusters are initialized with -- existing clusters will remain unaffected. But it might be worth highlighting this change more prominently to our users for added scrutiny.

irfansharif

TFTR!

bors r+

craig · 2023-01-06T13:33:27Z

Build succeeded:

Bazel Essential CI (Cockroach)

irfansharif requested review from ajwerner and dt December 16, 2022 23:39

irfansharif requested a review from a team as a code owner December 16, 2022 23:39

irfansharif requested a review from a team December 16, 2022 23:39

irfansharif requested a review from a team as a code owner December 16, 2022 23:39

irfansharif force-pushed the 221215.lower-gc-ttl branch from a0e56b9 to 6c471eb Compare December 16, 2022 23:41

irfansharif force-pushed the 221215.lower-gc-ttl branch from 6c471eb to 4b0f8c2 Compare December 19, 2022 15:29

irfansharif requested a review from a team as a code owner December 19, 2022 15:29

irfansharif commented Dec 19, 2022

View reviewed changes

irfansharif force-pushed the 221215.lower-gc-ttl branch from 4b0f8c2 to 89b9b38 Compare December 19, 2022 17:13

irfansharif force-pushed the 221215.lower-gc-ttl branch from 89b9b38 to 9c1ee03 Compare December 19, 2022 17:17

irfansharif requested a review from adityamaru January 4, 2023 16:00

irfansharif force-pushed the 221215.lower-gc-ttl branch from 9c1ee03 to 38dca99 Compare January 5, 2023 17:36

irfansharif requested a review from a team as a code owner January 5, 2023 17:36

irfansharif requested review from herkolategan and renatolabs and removed request for a team January 5, 2023 17:36

irfansharif commented Jan 5, 2023

View reviewed changes

irfansharif force-pushed the 221215.lower-gc-ttl branch 2 times, most recently from d4a1a44 to 1dba482 Compare January 5, 2023 18:38

adityamaru approved these changes Jan 6, 2023

View reviewed changes

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

irfansharif force-pushed the 221215.lower-gc-ttl branch from 1dba482 to 1d01816 Compare January 6, 2023 12:40

irfansharif commented Jan 6, 2023

View reviewed changes

craig bot merged commit 8921f1a into cockroachdb:master Jan 6, 2023

irfansharif deleted the 221215.lower-gc-ttl branch January 6, 2023 15:30

This was referenced Jan 7, 2023

PR #93836 - *: lower default GC TTL to 4h (1 of 2) cockroachdb/docs#15942

Closed

PR #93836 - *: lower default GC TTL to 4h (2 of 2) cockroachdb/docs#15943

Closed

rmloveland mentioned this pull request Mar 16, 2023

Update all GC TTL and range size mentions to DRY cockroachdb/docs#16506

Merged

Conversation

irfansharif commented Dec 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Dec 16, 2022

Uh oh!

knz commented Dec 17, 2022

Uh oh!

irfansharif left a comment

Choose a reason for hiding this comment

Uh oh!

irfansharif commented Dec 19, 2022

Uh oh!

irfansharif commented Jan 3, 2023

Uh oh!

postamar commented Jan 3, 2023

Uh oh!

irfansharif commented Jan 4, 2023

Uh oh!

adityamaru commented Jan 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

postamar commented Jan 5, 2023

Uh oh!

irfansharif left a comment

Choose a reason for hiding this comment

Uh oh!

adityamaru left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

irfansharif left a comment

Choose a reason for hiding this comment

Uh oh!

craig bot commented Jan 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

irfansharif commented Dec 16, 2022 •

edited

Loading

adityamaru commented Jan 5, 2023 •

edited

Loading