backupccl,spanconfig,kvserver: ExportRequest noops on ephemeral ranges by adityamaru · Pull Request #75451 · cockroachdb/cockroach

adityamaru · 2022-01-24T17:58:45Z

This change is the first of two changes that gets us to the goal of backup
ignoring certain table row data, and not holding up GC on these ranges.

This change does a few things:

It sets up the transport of the exclude_data_from_backup bit set on a
table descriptor, to the span configuration applied in KV.
It teaches ExportRequest on a range marked as excluded to return
an empty ExportResponse. In this way, a backup processor will receive no row
data to backup up for an ephemeral table.
A follow up change will also teach the SQLTranslator
to not populate the protected timestamp field on the SpanConfig for such
tables. This way, a long running backup will not hold up GC on such high-churn
tables. With no protection on such ranges, it is possible that an
ExportRequest targetting the range has a StartTime
below the range's GCThreshold. To avoid the returned BatchTimestampBeforeGCError
from failing the backup we decorate the the error with information about the
range being excluded from backup and handle the error in the backup processor.

Informs: #73536

Release note (sql change): BACKUP of a table marked with exclude_data_from_backup
via ALTER TABLE ... SET (exclude_data_from_backup = true) will no longer backup
that table's row data. The backup will continue to backup the table's descriptor
and related metadata, and so on restore we will end up with an empty version of
the backed up table.

cockroach-teamcity · 2022-01-24T17:58:55Z

This change is

pkg/ccl/backupccl/backup_test.go

adityamaru · 2022-01-24T18:01:07Z

first two commits are from #75295.

adityamaru · 2022-01-28T01:58:21Z

Please holdoff on reviewing this. I'll ping the PR once it is RFAL 🙂

adityamaru · 2022-02-03T03:40:37Z

@dt @irfansharif reworked this a bit with the new syntax, and with better tests. This is RFAL now!

adityamaru · 2022-02-07T14:18:38Z

friendly ping!

irfansharif

LGTM but you're the expert on the backupccl tests.

pkg/roachpb/span_config.proto

pkg/roachpb/errors.proto

pkg/kv/kvserver/replica.go

irfansharif · 2022-02-08T14:41:29Z

pkg/ccl/backupccl/backup_processor.go

+							// backup, it is safe to ignore the error. It is likely that the
+							// table has been configured with a low GC TTL, and so the data
+							// the backup is targeting has already been gc'ed.
+							if batchTimestampBeforeGCError.ExcludeDataFromBackup {


Noting for posterity: was a bit surprised to see this bubble up through the roachpb.BatchTimestampBeforeGCError but it seems the least invasive way given the GC threshold checks happen before command eval.

irfansharif · 2022-02-08T14:43:10Z

pkg/ccl/backupccl/backup_test.go

+		return startKey
+	}
+
+	getStoreAndReplica := func() (*kvserver.Store, *kvserver.Replica) {


Should some of these helpers be moved outside the test? We're re-using them in a few places (both in this PR, and IIRC other tests in the package).

Moved all these shared helpers to a utils file.

dt · 2022-02-09T01:33:17Z

pkg/ccl/backupccl/backup_test.go

+		return startKey
+	}
+
+	getStoreAndReplica := func() (*kvserver.Store, *kvserver.Replica) {


pkg/ccl/backupccl/backup_test.go

adityamaru · 2022-02-09T21:15:40Z

The increased diffs are just un-exporting some test utility methods that lint was complaining about. I consolidated helpers_test.go into testutils.go and so I think lint started complaining about exported fields without comments. There was no need for these to be exported since their use is limited to backupccl.

adityamaru · 2022-02-13T15:02:21Z

TFTR!

bors r=irfansharif,dt

craig · 2022-02-13T15:33:10Z

Build failed:

GitHub CI (Cockroach)

adityamaru · 2022-02-13T15:45:50Z

bors r-

… from backup This change is the first of two changes that gets us to the goal of backup ignoring certain table row data, and not holding up GC on these ranges. This change does a few things: - It sets up the transport of the exclude_data_from_backup bit set on a table descriptor, to the span configuration applied in KV. - It teaches ExportRequest on a range marked as excluded to return an empty ExportResponse. In this way, a backup processor will receive no row data to backup up for an ephemeral table. - A follow up change will also teach the SQLTranslator to not populate the protected timestamp field on the SpanConfig for such tables. This way, a long running backup will not hold up GC on such high-churn tables. With no protection on such ranges, it is possible that an ExportRequest targetting the range has a StartTime below the range's GCThreshold. To avoid the returned BatchTimestampBeforeGCError from failing the backup we decorate the the error with information about the range being excluded from backup and handle the error in the backup processor. Informs: cockroachdb#73536 Release note (sql change): BACKUP of a table marked with `exclude_data_from_backup` via `ALTER TABLE ... SET (exclude_data_from_backup = true)` will no longer backup that table's row data. The backup will continue to backup the table's descriptor and related metadata, and so on restore we will end up with an empty version of the backed up table.

adityamaru · 2022-02-14T16:08:03Z

bors r+

75451: backupccl,spanconfig,kvserver: ExportRequest noops on ephemeral ranges r=adityamaru a=adityamaru This change is the first of two changes that gets us to the goal of backup ignoring certain table row data, and not holding up GC on these ranges. This change does a few things: - It sets up the transport of the exclude_data_from_backup bit set on a table descriptor, to the span configuration applied in KV. - It teaches ExportRequest on a range marked as excluded to return an empty ExportResponse. In this way, a backup processor will receive no row data to backup up for an ephemeral table. - A follow up change will also teach the SQLTranslator to not populate the protected timestamp field on the SpanConfig for such tables. This way, a long running backup will not hold up GC on such high-churn tables. With no protection on such ranges, it is possible that an ExportRequest targetting the range has a StartTime below the range's GCThreshold. To avoid the returned BatchTimestampBeforeGCError from failing the backup we decorate the the error with information about the range being excluded from backup and handle the error in the backup processor. Informs: #73536 Release note (sql change): BACKUP of a table marked with `exclude_data_from_backup` via `ALTER TABLE ... SET (exclude_data_from_backup = true)` will no longer backup that table's row data. The backup will continue to backup the table's descriptor and related metadata, and so on restore we will end up with an empty version of the backed up table. 76168: ui,server: Table stats collection details added to Database pages r=ericharmeling a=ericharmeling Closes: #67510 Release justification: low-risk, high benefit changes to existing functionality Release note (ui change): - Added status of automatic statistics collection to the Database and Database table pages on the DB Console. - Added timestamp of last statistics collection to the Database details and Database table pages on the DB Console. Here's a video of the changes in the UI: https://user-images.githubusercontent.com/27286675/153521703-9ed19ed0-5fe9-4cb6-a537-1d227aeb0a0b.mov 76444: bazel: upgrade to bazel 5.0.0 r=rail a=rickystewart Closes #75796. Release note: None Co-authored-by: Aditya Maru <adityamaru@gmail.com> Co-authored-by: Eric Harmeling <eric.harmeling@cockroachlabs.com> Co-authored-by: Ricky Stewart <ricky@cockroachlabs.com>

craig · 2022-02-14T17:10:20Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2022-02-14T20:43:48Z

Build succeeded:

GitHub CI (Cockroach)

…_backup` to SpanConfig This change is a follow up to cockroachdb#75451 which taught ExportRequests to noop on ranges marked as `exclude_data_from_backup`. This change adds a field to the ProtectionPolicy in the SpanConfig. This field is set to true if the ProtectionPolicy can be ignored if the span it applies to has been marked to be excluded from backups i.e. the applied SpanConfig has `exclude_data_from_backup = true`. This field is currently only set to true when a protected timestamp record has been written by a backup job. This is to ensure that ProtectionPolicies written by non-backup users (CDC, streaming) on spans marked as `exclude_data_from_backup` are still respected when making GC decisions on the span. The work to teach the GC queue about this field will be done in a final follow up. Informs: cockroachdb#73536 Release note: None

…backup to SpanConfig This change is a follow up to cockroachdb#75451 which taught ExportRequests to noop on ranges marked as exclude_data_from_backup. This change This diff does two things: - It adds an `ignore_if_excluded_from_backup` bit to ptpb.Target that is set on PTS records written by backup schedules and jobs. - It adds an `ignore_if_excluded_from_backup` bit to the ProtectionPolicy that is shipped to KV as part of the SpanConfig. In a follow up PR, this bit on the SpanConfig will be used in conjunction with `exclude_data_from_backup` to decide whether or not to ignore the ProtectionPolicy when making GC decisions on a span. All other consumers of PTS records will default to setting this bit to false, and so their ProtectionPolicies will always influence GC even if `exclude_data_from_backup` is set to true. Informs: cockroachdb#73536 Release note: None

76831: spanconfigsqltranslator,jobsprotectedts: add `ignore_if_excluded_from_backup` to SpanConfig r=dt,irfansharif a=adityamaru This change is a follow up to #75451 which taught ExportRequests to noop on ranges marked as exclude_data_from_backup. This change This diff does two things: - It adds an `ignore_if_excluded_from_backup` bit to ptpb.Target that is set on PTS records written by backup schedules and jobs. - It adds an `ignore_if_excluded_from_backup` bit to the ProtectionPolicy that is shipped to KV as part of the SpanConfig. In a follow up PR, this bit on the SpanConfig will be used in conjunction with `exclude_data_from_backup` to decide whether or not to ignore the ProtectionPolicy when making GC decisions on a span. All other consumers of PTS records will default to setting this bit to false, and so their ProtectionPolicies will always influence GC even if `exclude_data_from_backup` is set to true. Informs: #73536 Release note: None Co-authored-by: Aditya Maru <adityamaru@gmail.com>

…backup to SpanConfig This change is a follow up to cockroachdb#75451 which taught ExportRequests to noop on ranges marked as exclude_data_from_backup. This change This diff does two things: - It adds an `ignore_if_excluded_from_backup` bit to ptpb.Target that is set on PTS records written by backup schedules and jobs. - It adds an `ignore_if_excluded_from_backup` bit to the ProtectionPolicy that is shipped to KV as part of the SpanConfig. In a follow up PR, this bit on the SpanConfig will be used in conjunction with `exclude_data_from_backup` to decide whether or not to ignore the ProtectionPolicy when making GC decisions on a span. All other consumers of PTS records will default to setting this bit to false, and so their ProtectionPolicies will always influence GC even if `exclude_data_from_backup` is set to true. Informs: cockroachdb#73536 Release note: None

adityamaru requested review from a team, dt and irfansharif January 24, 2022 17:58

adityamaru requested a review from a team as a code owner January 24, 2022 17:58

adityamaru requested a review from a team January 24, 2022 17:58

adityamaru commented Jan 24, 2022

View reviewed changes

pkg/ccl/backupccl/backup_test.go Outdated Show resolved Hide resolved

adityamaru mentioned this pull request Jan 28, 2022

sql: Add exclude_data_from_backup storage option to ALTER TABLE #75295

Merged

adityamaru mentioned this pull request Jan 31, 2022

sql: persist TTL metadata to the descriptor #75602

Merged

adityamaru force-pushed the add-pts-ephemeral branch from 9439375 to 2d395cb Compare February 3, 2022 03:28

adityamaru removed request for a team February 3, 2022 03:29

adityamaru force-pushed the add-pts-ephemeral branch from 2d395cb to 1bab752 Compare February 3, 2022 03:39

adityamaru force-pushed the add-pts-ephemeral branch from 1bab752 to 1fd9303 Compare February 3, 2022 21:27

stevendanna added the T-disaster-recovery label Feb 7, 2022

irfansharif approved these changes Feb 8, 2022

View reviewed changes

dt approved these changes Feb 9, 2022

View reviewed changes

adityamaru force-pushed the add-pts-ephemeral branch 5 times, most recently from 52472d2 to e4da251 Compare February 9, 2022 21:14

adityamaru force-pushed the add-pts-ephemeral branch 3 times, most recently from 7c466e7 to 7eb6024 Compare February 12, 2022 22:25

adityamaru force-pushed the add-pts-ephemeral branch from 7eb6024 to a2aad65 Compare February 13, 2022 15:53

craig bot merged commit 07a3683 into cockroachdb:master Feb 14, 2022

cockroach-teamcity mentioned this pull request Feb 14, 2022

backupccl,spanconfig,kvserver: ExportRequest noops on ephemeral ranges cockroachdb/docs#13022

Closed

adityamaru deleted the add-pts-ephemeral branch February 14, 2022 21:50

adityamaru mentioned this pull request Feb 20, 2022

spanconfigsqltranslator,jobsprotectedts: add ignore_if_excluded_from_backup to SpanConfig #76831

Merged

Conversation

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

Uh oh!

Uh oh!

cockroach-teamcity commented Jan 24, 2022

Uh oh!

Uh oh!

adityamaru commented Jan 24, 2022

Uh oh!

adityamaru commented Jan 28, 2022

Uh oh!

adityamaru commented Feb 3, 2022

Uh oh!

adityamaru commented Feb 7, 2022

Uh oh!

irfansharif left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

irfansharif Feb 8, 2022

Choose a reason for hiding this comment

Uh oh!

irfansharif Feb 8, 2022

Choose a reason for hiding this comment

Uh oh!

dt Feb 9, 2022

Choose a reason for hiding this comment

Uh oh!

adityamaru Feb 9, 2022

Choose a reason for hiding this comment

Uh oh!

dt Feb 9, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adityamaru commented Feb 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adityamaru commented Feb 13, 2022

Uh oh!

craig bot commented Feb 13, 2022

Uh oh!

adityamaru commented Feb 13, 2022

Uh oh!

adityamaru commented Feb 14, 2022

Uh oh!

craig bot commented Feb 14, 2022

Uh oh!

craig bot commented Feb 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

adityamaru commented Jan 24, 2022 •

edited

Loading

adityamaru commented Feb 9, 2022 •

edited

Loading