-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
systemschema: use ExcludeDataFromBackup on some tables #119947
systemschema: use ExcludeDataFromBackup on some tables #119947
Conversation
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
b9d27cc
to
de39d70
Compare
Hey @rafiss, |
It cannot be backported, since modifying system tables in this way is not possible in patch releases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just one minor comment
Reviewed 23 of 23 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @rafiss)
pkg/upgrade/upgrades/v24_1_system_database.go
line 111 at r1 (raw file):
if err := deps.DB.DescsTxn(ctx, func(ctx context.Context, txn descs.Txn) error { for _, tabName := range []catconstants.SystemTableName{ catconstants.ReplicationConstraintStatsTableName,
Do we want an upgrade test to confirm this?
Certain tables are updated frequently and contain data that should never be backed up or restored. Allowing backups on them is not desired, since that results in protected timestamps being applied on the table, which prevents GC of old data. This commit excludes the lease, replication_constraint_stats, replication_stats, and tenant_usage tables from backups. These are the tables that explicitly have their gc.ttl configured to a lower value. Release note: None
de39d70
to
3d87bec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @fqazi)
pkg/upgrade/upgrades/v24_1_system_database.go
line 111 at r1 (raw file):
Previously, fqazi (Faizan Qazi) wrote…
Do we want an upgrade test to confirm this?
i added a logic test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 4 files at r2, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained
tftr! bors r+ |
Build succeeded: |
Certain tables are updated frequently and contain data that should never be backed up or restored. Allowing backups on them is not desired, since that results in protected timestamps being applied on the table, which prevents GC of old data.
This commit excludes the lease, replication_constraint_stats, replication_stats, and tenant_usage tables from backups. These are the tables that explicitly have their gc.ttl configured to a lower value.
informs: #80866
Release note: None