Skip to content
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.

Sign up for GitHub

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

backupccl: add deprecation notice for BACKUP TO and RESTORE FROM without subdir #78165

Merged
merged 3 commits into from
Mar 22, 2022
+121 −68
Merged

backupccl: add deprecation notice for BACKUP TO and RESTORE FROM without subdir #78165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9118a0f
backupccl: add deprecation notice for `BACKUP TO`
adityamaru Mar 21, 2022
45288c7
backupccl: add deprecation notice to `RESTORE FROM` without subdir
adityamaru Mar 21, 2022
548c5e4
backupccl: add datadriven tests for deprecation notice
adityamaru Mar 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions pkg/ccl/backupccl/backup_planning.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgnotice"
"github.com/cockroachdb/cockroach/pkg/sql/privilege"
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
"github.com/cockroachdb/cockroach/pkg/util/hlc"
Expand Down Expand Up @@ -592,6 +593,16 @@ func backupPlanHook(
return nil, nil, nil, false, err
}

// Deprecation notice for `BACKUP TO` syntax. Remove this once the syntax is
// deleted in 22.2.
if !backupStmt.Nested {
p.BufferClientNotice(ctx,
pgnotice.Newf("The `BACKUP TO` syntax will be removed in a future release, please"+
" switch over to using `BACKUP INTO` to create a backup collection: %s. "+
"Backups created using the `BACKUP TO` syntax may not be restoreable in the next major version release.",
"https://www.cockroachlabs.com/docs/stable/backup.html#considerations"))
}

var err error
subdirFn := func() (string, error) { return "", nil }
if backupStmt.Subdir != nil {
Expand Down
7 changes: 7 additions & 0 deletions pkg/ccl/backupccl/restore_planning.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1075,6 +1075,13 @@ func restorePlanHook(
if err != nil {
return nil, nil, nil, false, err
}
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before the 22.1 release, I think we'll need to add a docs page that provides step by step guidance on restoring backups with the new restore syntax that were originally created with the old backup syntax. That page should be linked to this warning.

Copy link
Contributor Author

@adityamaru adityamaru Mar 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of a section about this, but I think it can either be inline where this link points to today as a notice/warning, or be linked out from there. cc: @kathancox

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw, It seems like inline would be a good idea. But, I also think expanding the deprecation notice and recording this in the docs — especially if workarounds are needed if a backup was taken with BACKUP TO, but would need to be restored with the new RESTORE syntax is important. When the docs issue comes through for this with the release notes, I'll prioritize into 22.1 work.

Copy link
Member

@dt dt Mar 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if we need workarounds: recall RESTORE only supports restoring backups from the previous major version or newer, so if 22.1 deprecates backup to (but can still restore backups made with it, including by 21.2), then 22.2 can just drop support and we don't need to supply workarounds, since any 22.1-made backups, the oldest it should need to restore, should be in collections anyway.

Copy link
Collaborator

@msbutler msbutler Mar 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adityamaru do you know if there's a clean migration from old backups that use INCREMENTAL FROM to the new incremental_location parameter? And how that interacts with locality aware backups? Aside from this, I think the user migration is pretty straightforward. Perhaps a data driven test in this PR could validate the different migration stories.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps then we should make the BACKUP TO warning even scarrier: "your backup will not be restorable in the next major release"

Copy link
Contributor Author

@adityamaru adityamaru Mar 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it is restorable right?

BACKUP TABLE foo TO 'nodelocal://1/bar' can be restored using RESTORE FROM 'bar' IN 'nodelocal://1/';? The x IN y just appends the path to y/x and looks for the manifest to restore.

I agree that we should have some documentation helping customers in 22.2 restoring backups written in 22.1 using deprecated syntax, but thats docs issue that can be addressed once this is merged?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that happy path is restorable, but there may be other user backup file configurations that may not be (e.g. ones that use INCREMENTAL FROM or locality aware backups). But i'm not sure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it is restorable right?

technically yes, but I'd just as soon sidestep this whole mixing old and new and just say it isn't.

Copy link
Contributor Author

@adityamaru adityamaru Mar 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a slightly sterner warning to the backup code, let me know what yall think.

// Deprecation notice for non-colelction `RESTORE FROM` syntax. Remove this
// once the syntax is deleted in 22.2.
p.BufferClientNotice(ctx,
pgnotice.Newf("The `RESTORE FROM <backup>` syntax will be removed in a future release, please"+
" switch over to using `RESTORE FROM <backup> IN <collection>` to restore a particular backup from a collection: %s",
"https://www.cockroachlabs.com/docs/stable/restore.html#view-the-backup-subdirectories"))
}

var incStorageFn func() ([]string, error)
Expand Down
22 changes: 11 additions & 11 deletions pkg/ccl/backupccl/testdata/backup-restore/backup-permissions
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,14 @@ INSERT INTO d.t VALUES (1), (2), (3);

# BACKUP is not allowed in a batch-statement.
exec-sql
BACKUP TO 'nodelocal://0/test-root/';
BACKUP INTO 'nodelocal://0/test-root/';
SELECT 1;
----
pq: BACKUP cannot be used inside a multi-statement transaction without DETACHED option

# Cluster backup should succeed as a root user.
exec-sql
BACKUP TO 'nodelocal://0/test-root/'
BACKUP INTO 'nodelocal://0/test-root/'
----

# Backups should succeed as a non-root user with admin role.
Expand All @@ -28,15 +28,15 @@ GRANT ADMIN TO testuser;
----

exec-sql user=testuser
BACKUP TO 'nodelocal://0/test-nonroot-cluster';
BACKUP INTO 'nodelocal://0/test-nonroot-cluster';
----

exec-sql user=testuser
BACKUP DATABASE d TO 'nodelocal://0/test-nonroot-db';
BACKUP DATABASE d INTO 'nodelocal://0/test-nonroot-db';
----

exec-sql user=testuser
BACKUP TABLE d.t TO 'nodelocal://0/test-nonroot-table';
BACKUP TABLE d.t INTO 'nodelocal://0/test-nonroot-table';
----

# Start a new cluster with the same IO dir.
Expand Down Expand Up @@ -75,7 +75,7 @@ GRANT CONNECT ON DATABASE d2 TO testuser;

# Table backup as a non-admin user should have SELECT privileges.
exec-sql user=testuser
BACKUP TABLE d2.t TO 'nodelocal://0/d2-table'
BACKUP TABLE d2.t INTO 'nodelocal://0/d2-table'
----
pq: user testuser does not have SELECT privilege on relation t

Expand All @@ -90,7 +90,7 @@ CREATE SCHEMA sc2;

# Schema backup as a non-admin user should have USAGE privileges.
exec-sql user=testuser
BACKUP DATABASE d2 TO 'nodelocal://0/d2-schema';
BACKUP DATABASE d2 INTO 'nodelocal://0/d2-schema';
----
pq: user testuser does not have USAGE privilege on schema sc2

Expand All @@ -108,7 +108,7 @@ REVOKE USAGE ON TYPE d2.greeting FROM public;

# Type backup as a non-admin user should have USAGE privileges.
exec-sql user=testuser
BACKUP DATABASE d2 TO 'nodelocal://0/d2-schema';
BACKUP DATABASE d2 INTO 'nodelocal://0/d2-schema';
----
pq: user testuser does not have USAGE privilege on type greeting

Expand All @@ -118,11 +118,11 @@ GRANT USAGE ON TYPE d2.greeting TO testuser;

# testuser should now have all the required privileges.
exec-sql server=s2 user=testuser
BACKUP DATABASE d2 TO 'nodelocal://0/d2';
BACKUP DATABASE d2 INTO 'nodelocal://0/d2';
----

exec-sql server=s2 user=testuser
BACKUP TABLE d2.t TO 'nodelocal://0/d2-table';
BACKUP TABLE d2.t INTO 'nodelocal://0/d2-table';
----

exec-sql server=s2 user=testuser
Expand All @@ -147,7 +147,7 @@ SHOW BACKUP 'http://COCKROACH_TEST_HTTP_SERVER/'
pq: only users with the admin role are allowed to SHOW BACKUP from the specified http URI

exec-sql user=testuser
BACKUP DATABASE d TO 'nodelocal://0/test3'
BACKUP DATABASE d INTO 'nodelocal://0/test3'
----
pq: only users with the admin role are allowed to BACKUP to the specified nodelocal URI

Expand Down
4 changes: 2 additions & 2 deletions pkg/ccl/backupccl/testdata/backup-restore/column-families
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,15 @@ SET CLUSTER SETTING bulkio.backup.merge_file_buffer_size = '1MiB';
----

exec-sql
BACKUP cfs TO 'nodelocal://1/foo';
BACKUP cfs INTO 'nodelocal://1/foo';
----

exec-sql
CREATE DATABASE r1;
----

exec-sql
RESTORE cfs FROM 'nodelocal://1/foo' WITH into_db='r1';
RESTORE cfs FROM LATEST IN 'nodelocal://1/foo' WITH into_db='r1';
----

query-sql
Expand Down
8 changes: 4 additions & 4 deletions pkg/ccl/backupccl/testdata/backup-restore/descriptor-broadening
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,19 +11,19 @@ CREATE TABLE db1.t (a INT);
----

exec-sql
BACKUP DATABASE db1 TO 'nodelocal://1/backup';
BACKUP DATABASE db1 INTO 'nodelocal://1/backup';
----

exec-sql
BACKUP DATABASE db1,db2 TO 'nodelocal://1/backup';
BACKUP DATABASE db1,db2 INTO LATEST IN 'nodelocal://1/backup';
----
pq: previous backup does not contain the complete database "db2"

exec-sql
BACKUP db1.t TO 'nodelocal://1/backup_2';
BACKUP db1.t INTO 'nodelocal://1/backup_2';
----

exec-sql
BACKUP DATABASE db1 TO 'nodelocal://1/backup_2';
BACKUP DATABASE db1 INTO LATEST IN 'nodelocal://1/backup_2';
----
pq: previous backup does not contain the complete database "db1"
43 changes: 39 additions & 4 deletions pkg/ccl/backupccl/testdata/backup-restore/feature-flags
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
subtest backup-feature-flags

new-server name=s1
----

Expand All @@ -13,7 +15,7 @@ SET CLUSTER SETTING feature.backup.enabled = FALSE;
----

exec-sql
BACKUP TO 'nodelocal://0/test-root/';
BACKUP INTO 'nodelocal://0/test-root/';
----
pq: feature BACKUP was disabled by the database administrator

Expand All @@ -23,7 +25,7 @@ SET CLUSTER SETTING feature.backup.enabled = TRUE;
----

exec-sql
BACKUP TO 'nodelocal://0/test-root/';
BACKUP INTO 'nodelocal://0/test-root/';
----

exec-sql
Expand All @@ -36,7 +38,7 @@ SET CLUSTER SETTING feature.restore.enabled = FALSE;
----

exec-sql
RESTORE TABLE d.t FROM 'nodelocal://0/test-root/';
RESTORE TABLE d.t FROM LATEST IN 'nodelocal://0/test-root/';
----
pq: feature RESTORE was disabled by the database administrator

Expand All @@ -46,5 +48,38 @@ SET CLUSTER SETTING feature.restore.enabled = TRUE;
----

exec-sql
RESTORE TABLE d.t FROM 'nodelocal://0/test-root/';
RESTORE TABLE d.t FROM LATEST IN 'nodelocal://0/test-root/';
----

subtest end

# TODO(adityamaru): Delete in 22.2 once the syntax is removed. We don't want
# to start a testserver just for this test.
subtest backup-restore-deprecation-notice

exec-sql
BACKUP TO 'nodelocal://1/deprecated';
----
NOTICE: The `BACKUP TO` syntax will be removed in a future release, please switch over to using `BACKUP INTO` to create a backup collection: https://www.cockroachlabs.com/docs/stable/backup.html#considerations. Backups created using the `BACKUP TO` syntax may not be restoreable in the next major version release.

exec-sql
BACKUP TO 'nodelocal://1/deprecated/incfrom' INCREMENTAL FROM 'nodelocal://1/deprecated';
----
NOTICE: The `BACKUP TO` syntax will be removed in a future release, please switch over to using `BACKUP INTO` to create a backup collection: https://www.cockroachlabs.com/docs/stable/backup.html#considerations. Backups created using the `BACKUP TO` syntax may not be restoreable in the next major version release.

exec-sql
DROP TABLE d.t;
----

exec-sql
RESTORE TABLE d.t FROM 'nodelocal://1/deprecated';
----
NOTICE: The `RESTORE FROM <backup>` syntax will be removed in a future release, please switch over to using `RESTORE FROM <backup> IN <collection>` to restore a particular backup from a collection: https://www.cockroachlabs.com/docs/stable/restore.html#view-the-backup-subdirectories


exec-sql
RESTORE SYSTEM USERS FROM 'nodelocal://1/deprecated';
----
NOTICE: The `RESTORE FROM <backup>` syntax will be removed in a future release, please switch over to using `RESTORE FROM <backup> IN <collection>` to restore a particular backup from a collection: https://www.cockroachlabs.com/docs/stable/restore.html#view-the-backup-subdirectories

subtest end
4 changes: 2 additions & 2 deletions pkg/ccl/backupccl/testdata/backup-restore/max-row-size
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,15 @@ INSERT INTO maxrow VALUES (2, repeat('x', 20000))
pq: row larger than max row size: table 109 family 0 primary key /Table/109/1/2/0 size 20013

exec-sql
BACKUP maxrow TO 'nodelocal://1/maxrow';
BACKUP maxrow INTO 'nodelocal://1/maxrow';
----

exec-sql
CREATE DATABASE d2;
----

exec-sql
RESTORE maxrow FROM 'nodelocal://1/maxrow' WITH into_db='d2';
RESTORE maxrow FROM LATEST IN 'nodelocal://1/maxrow' WITH into_db='d2';
----

query-sql
Expand Down
26 changes: 13 additions & 13 deletions pkg/ccl/backupccl/testdata/backup-restore/multiregion
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,27 +15,27 @@ us-east-1
us-west-1

exec-sql
BACKUP DATABASE d TO 'nodelocal://1/database_backup/';
BACKUP DATABASE d INTO 'nodelocal://1/database_backup/';
----

exec-sql
BACKUP TO 'nodelocal://1/full_cluster_backup/';
BACKUP INTO 'nodelocal://1/full_cluster_backup/';
----

# A new cluster with the same locality settings.
new-server name=s2 share-io-dir=s1 allow-implicit-access localities=us-east-1,us-west-1,eu-central-1
----

exec-sql
RESTORE FROM 'nodelocal://0/full_cluster_backup/';
RESTORE FROM LATEST IN 'nodelocal://0/full_cluster_backup/';
----

exec-sql
DROP DATABASE d;
----

exec-sql
RESTORE DATABASE d FROM 'nodelocal://0/database_backup/';
RESTORE DATABASE d FROM LATEST IN 'nodelocal://0/database_backup/';
----

query-sql
Expand All @@ -52,13 +52,13 @@ new-server name=s3 share-io-dir=s1 allow-implicit-access localities=eu-central-1
----

exec-sql
RESTORE DATABASE d FROM 'nodelocal://0/database_backup/';
RESTORE DATABASE d FROM LATEST IN 'nodelocal://0/database_backup/';
----
pq: detected a mismatch in regions between the restore cluster and the backup cluster, missing regions detected: us-east-1, us-west-1.
HINT: there are two ways you can resolve this issue: 1) update the cluster to which you're restoring to ensure that the regions present on the nodes' --locality flags match those present in the backup image, or 2) restore with the "skip_localities_check" option

exec-sql
RESTORE FROM 'nodelocal://0/full_cluster_backup/';
RESTORE FROM LATEST IN 'nodelocal://0/full_cluster_backup/';
----
pq: detected a mismatch in regions between the restore cluster and the backup cluster, missing regions detected: us-east-1, us-west-1.
HINT: there are two ways you can resolve this issue: 1) update the cluster to which you're restoring to ensure that the regions present on the nodes' --locality flags match those present in the backup image, or 2) restore with the "skip_localities_check" option
Expand All @@ -77,11 +77,11 @@ INSERT INTO no_region_db_2.t VALUES (1), (2), (3);
----

exec-sql
BACKUP DATABASE no_region_db TO 'nodelocal://1/no_region_database_backup/';
BACKUP DATABASE no_region_db INTO 'nodelocal://1/no_region_database_backup/';
----

exec-sql
BACKUP TO 'nodelocal://1/no_region_cluster_backup/';
BACKUP INTO 'nodelocal://1/no_region_cluster_backup/';
----

exec-sql
Expand All @@ -97,7 +97,7 @@ SET CLUSTER SETTING sql.defaults.primary_region = 'non-existent-region';
----

exec-sql
RESTORE DATABASE no_region_db FROM 'nodelocal://1/no_region_database_backup/';
RESTORE DATABASE no_region_db FROM LATEST IN 'nodelocal://1/no_region_database_backup/';
----
pq: region "non-existent-region" does not exist
HINT: valid regions: eu-central-1, eu-north-1
Expand All @@ -109,7 +109,7 @@ SET CLUSTER SETTING sql.defaults.primary_region = 'eu-central-1';
----

exec-sql
RESTORE DATABASE no_region_db FROM 'nodelocal://1/no_region_database_backup/';
RESTORE DATABASE no_region_db FROM LATEST IN 'nodelocal://1/no_region_database_backup/';
----
NOTICE: setting the PRIMARY REGION as eu-central-1 on database no_region_db
HINT: to change the default primary region, use SET CLUSTER SETTING sql.defaults.primary_region = 'region' or use RESET CLUSTER SETTING sql.defaults.primary_region to disable this behavior
Expand All @@ -136,7 +136,7 @@ INSERT INTO eu_central_db.t VALUES (1), (2), (3);
NOTICE: setting eu-central-1 as the PRIMARY REGION as no PRIMARY REGION was specified

exec-sql
BACKUP DATABASE eu_central_db TO 'nodelocal://1/eu_central_database_backup/';
BACKUP DATABASE eu_central_db INTO 'nodelocal://1/eu_central_database_backup/';
----

# New cluster for a cluster backup.
Expand All @@ -148,7 +148,7 @@ SET CLUSTER SETTING sql.defaults.primary_region = 'eu-north-1';
----

exec-sql
RESTORE FROM 'nodelocal://1/no_region_cluster_backup/';
RESTORE FROM LATEST IN 'nodelocal://1/no_region_cluster_backup/';
----
NOTICE: setting the PRIMARY REGION as eu-north-1 on database defaultdb
HINT: to change the default primary region, use SET CLUSTER SETTING sql.defaults.primary_region = 'region' or use RESET CLUSTER SETTING sql.defaults.primary_region to disable this behavior
Expand Down Expand Up @@ -176,7 +176,7 @@ public t table root <nil> REGIONAL BY TABLE IN PRIMARY REGION

# Check we can restore without triggering the default primary region.
exec-sql
RESTORE DATABASE eu_central_db FROM 'nodelocal://1/eu_central_database_backup/';
RESTORE DATABASE eu_central_db FROM LATEST IN 'nodelocal://1/eu_central_database_backup/';
----

query-sql
Expand Down
Loading