Skip to content

Commit

Permalink
Merge #95562 #95745
Browse files Browse the repository at this point in the history 
95562: backupccl: explicitly parse SHOW BACKUP options r=benbardin a=msbutler

Fixes: #82912

Release note (sql change): Previously, SHOW BACKUP options would get parsed as
`kv_options`, which meant that a user could not pass multiple values to a show
backup option, causing feature gaps in SHOW BACKUP relative to BACKUP and
RESTORE. This patch rewrites the show backup option parser, closing the
following feature gaps:
 1. A user can now pass and check multiple KMS URIs in
SHOW BACKUP 
2. A user can pass locality aware incremental_locations, allowing a
user to also pass the check_files parameter to a locality aware backup chain
that also specifies the backup incremental location.

Note that while this patch introduces a couple new words to the CRDB SQL
syntax, the same SHOW BACKUP options should remain documented, specifically:
- [public option] -> value
- AS_JSON -> N/A
- CHECK_FILES -> N/A
- INCREMENTAL_LOCATION -> string, with potentially multiple uris
- DEBUG_IDS -> N/A
- KMS -> string, with potentially multiple uris
- PRIVILEGES -> N/A
- ENCRYPTION_PASSPHRASE -> string

95745: sql: remove redundant session iteration r=xinhaoz,yuzefovich a=ecwall

Fixes #95743

Improves session/query cancelation with the following
1) Replaces session scanning by session ID with map lookup.
2) Replaces active query scanning by query ID with map lookup
   (session containing query to cancel is still scanned for).
3) Does not serialize entire session to get session username or id.

Informs #77676

77676 was closed but some test cases incorrectly mentioned that addressing
77676 fixed them. This PR correctly fixes these test cases.

Release note: None


Co-authored-by: Michael Butler <[email protected]>
Co-authored-by: Evan Wall <[email protected]>
  • Loading branch information
@msbutler @ecwall
3 people committed Jan 27, 2023
3 parents b23943c + e6f47a0 + 129af11 commit 5f0c56e
Show file tree
Hide file tree
Showing 15 changed files with 580 additions and 392 deletions.
28 changes: 7 additions & 21 deletions docs/generated/sql/bnf/show_backup.bnf
View file
Original file line number Diff line number Diff line change
@@ -1,23 +1,9 @@
show_backup_stmt ::=
'SHOW' 'BACKUPS' 'IN' location_opt_list
| 'SHOW' 'BACKUP' show_backup_details 'FROM' string_or_placeholder 'IN' string_or_placeholder_opt_list 'WITH' kv_option_list
| 'SHOW' 'BACKUP' show_backup_details 'FROM' string_or_placeholder 'IN' string_or_placeholder_opt_list 'WITH' 'OPTIONS' '(' kv_option_list ')'
| 'SHOW' 'BACKUP' show_backup_details 'FROM' string_or_placeholder 'IN' string_or_placeholder_opt_list
| 'SHOW' 'BACKUP' subdirectory 'IN' location_opt_list 'WITH' kv_option_list
| 'SHOW' 'BACKUP' subdirectory 'IN' location_opt_list 'WITH' 'OPTIONS' '(' kv_option_list ')'
| 'SHOW' 'BACKUP' subdirectory 'IN' location_opt_list
| 'SHOW' 'BACKUP' string_or_placeholder 'WITH' kv_option_list
| 'SHOW' 'BACKUP' string_or_placeholder 'WITH' 'OPTIONS' '(' kv_option_list ')'
| 'SHOW' 'BACKUP' string_or_placeholder
| 'SHOW' 'BACKUP' 'SCHEMAS' location 'WITH' kv_option_list
| 'SHOW' 'BACKUP' 'SCHEMAS' location 'WITH' 'OPTIONS' '(' kv_option_list ')'
| 'SHOW' 'BACKUP' 'SCHEMAS' location
| 'SHOW' 'BACKUP' 'FILES' string_or_placeholder 'WITH' kv_option_list
| 'SHOW' 'BACKUP' 'FILES' string_or_placeholder 'WITH' 'OPTIONS' '(' kv_option_list ')'
| 'SHOW' 'BACKUP' 'FILES' string_or_placeholder
| 'SHOW' 'BACKUP' 'RANGES' string_or_placeholder 'WITH' kv_option_list
| 'SHOW' 'BACKUP' 'RANGES' string_or_placeholder 'WITH' 'OPTIONS' '(' kv_option_list ')'
| 'SHOW' 'BACKUP' 'RANGES' string_or_placeholder
| 'SHOW' 'BACKUP' 'VALIDATE' string_or_placeholder 'WITH' kv_option_list
| 'SHOW' 'BACKUP' 'VALIDATE' string_or_placeholder 'WITH' 'OPTIONS' '(' kv_option_list ')'
| 'SHOW' 'BACKUP' 'VALIDATE' string_or_placeholder
| 'SHOW' 'BACKUP' show_backup_details 'FROM' string_or_placeholder 'IN' string_or_placeholder_opt_list opt_with_show_backup_options
| 'SHOW' 'BACKUP' subdirectory 'IN' location_opt_list opt_with_show_backup_options
| 'SHOW' 'BACKUP' string_or_placeholder opt_with_show_backup_options
| 'SHOW' 'BACKUP' 'SCHEMAS' location opt_with_show_backup_options
| 'SHOW' 'BACKUP' 'FILES' string_or_placeholder opt_with_show_backup_options
| 'SHOW' 'BACKUP' 'RANGES' string_or_placeholder opt_with_show_backup_options
| 'SHOW' 'BACKUP' 'VALIDATE' string_or_placeholder opt_with_show_backup_options
45 changes: 37 additions & 8 deletions docs/generated/sql/bnf/stmt_block.bnf
View file
Original file line number Diff line number Diff line change
Expand Up @@ -771,13 +771,13 @@ use_stmt ::=

show_backup_stmt ::=
'SHOW' 'BACKUPS' 'IN' string_or_placeholder_opt_list
| 'SHOW' 'BACKUP' show_backup_details 'FROM' string_or_placeholder 'IN' string_or_placeholder_opt_list opt_with_options
| 'SHOW' 'BACKUP' string_or_placeholder 'IN' string_or_placeholder_opt_list opt_with_options
| 'SHOW' 'BACKUP' string_or_placeholder opt_with_options
| 'SHOW' 'BACKUP' 'SCHEMAS' string_or_placeholder opt_with_options
| 'SHOW' 'BACKUP' 'FILES' string_or_placeholder opt_with_options
| 'SHOW' 'BACKUP' 'RANGES' string_or_placeholder opt_with_options
| 'SHOW' 'BACKUP' 'VALIDATE' string_or_placeholder opt_with_options
| 'SHOW' 'BACKUP' show_backup_details 'FROM' string_or_placeholder 'IN' string_or_placeholder_opt_list opt_with_show_backup_options
| 'SHOW' 'BACKUP' string_or_placeholder 'IN' string_or_placeholder_opt_list opt_with_show_backup_options
| 'SHOW' 'BACKUP' string_or_placeholder opt_with_show_backup_options
| 'SHOW' 'BACKUP' 'SCHEMAS' string_or_placeholder opt_with_show_backup_options
| 'SHOW' 'BACKUP' 'FILES' string_or_placeholder opt_with_show_backup_options
| 'SHOW' 'BACKUP' 'RANGES' string_or_placeholder opt_with_show_backup_options
| 'SHOW' 'BACKUP' 'VALIDATE' string_or_placeholder opt_with_show_backup_options

show_columns_stmt ::=
'SHOW' 'COLUMNS' 'FROM' table_name with_comment
Expand Down Expand Up @@ -1010,6 +1010,7 @@ unreserved_keyword ::=
| 'ALTER'
| 'ALWAYS'
| 'ASENSITIVE'
| 'AS_JSON'
| 'AT'
| 'ATOMIC'
| 'ATTRIBUTE'
Expand All @@ -1032,6 +1033,7 @@ unreserved_keyword ::=
| 'CAPABILITY'
| 'CASCADE'
| 'CHANGEFEED'
| 'CHECK_FILES'
| 'CLOSE'
| 'CLUSTER'
| 'COLUMNS'
Expand Down Expand Up @@ -1069,7 +1071,9 @@ unreserved_keyword ::=
| 'DATABASES'
| 'DAY'
| 'DEALLOCATE'
| 'DEBUG_IDS'
| 'DEBUG_PAUSE_ON'
| 'DEBUG_DUMP_METADATA_SST'
| 'DECLARE'
| 'DELETE'
| 'DEFAULTS'
Expand All @@ -1087,6 +1091,7 @@ unreserved_keyword ::=
| 'ENCODING'
| 'ENCRYPTED'
| 'ENCRYPTION_PASSPHRASE'
| 'ENCRYPTION_INFO_DIR'
| 'ENUM'
| 'ENUMS'
| 'ESCAPE'
Expand Down Expand Up @@ -1901,6 +1906,11 @@ show_backup_details ::=
| 'RANGES'
| 'VALIDATE'

opt_with_show_backup_options ::=
'WITH' show_backup_options_list
| 'WITH' 'OPTIONS' '(' show_backup_options_list ')'
|

with_comment ::=
'WITH' 'COMMENT'
|
Expand Down Expand Up @@ -2642,6 +2652,9 @@ extra_var_value ::=
'ON'
| cockroachdb_extra_reserved_keyword

show_backup_options_list ::=
( show_backup_options ) ( ( ',' show_backup_options ) )*

targets_roles ::=
'ROLE' role_spec_list
| 'SCHEMA' schema_name_list
Expand Down Expand Up @@ -3181,6 +3194,17 @@ for_locking_item ::=
var_list ::=
( var_value ) ( ( ',' var_value ) )*

show_backup_options ::=
'AS_JSON'
| 'CHECK_FILES'
| 'DEBUG_IDS'
| 'INCREMENTAL_LOCATION' '=' string_or_placeholder_opt_list
| 'KMS' '=' string_or_placeholder_opt_list
| 'ENCRYPTION_PASSPHRASE' '=' string_or_placeholder
| 'PRIVILEGES'
| 'ENCRYPTION_INFO_DIR' '=' string_or_placeholder
| 'DEBUG_DUMP_METADATA_SST'

schema_wildcard ::=
wildcard_pattern

Expand Down Expand Up @@ -3382,11 +3406,16 @@ family_name ::=
name

bare_label_keywords ::=
'ATOMIC'
'AS_JSON'
| 'ATOMIC'
| 'CALLED'
| 'COST'
| 'CHECK_FILES'
| 'DEBUG_IDS'
| 'DEBUG_DUMP_METADATA_SST'
| 'DEFINER'
| 'DEPENDS'
| 'ENCRYPTION_INFO_DIR'
| 'EXTERNAL'
| 'IMMUTABLE'
| 'INPUT'
Expand Down
8 changes: 0 additions & 8 deletions pkg/ccl/backupccl/backup_planning.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,14 +62,6 @@ import (
)

const (
backupOptRevisionHistory = "revision_history"
backupOptWithPrivileges = "privileges"
backupOptAsJSON = "as_json"
backupOptWithDebugIDs = "debug_ids"
backupOptIncStorage = "incremental_location"
backupOptDebugMetadataSST = "debug_dump_metadata_sst"
backupOptEncDir = "encryption_info_dir"
backupOptCheckFiles = "check_files"
// backupPartitionDescriptorPrefix is the file name prefix for serialized
// BackupPartitionDescriptor protos.
backupPartitionDescriptorPrefix = "BACKUP_PART"
Expand Down
41 changes: 18 additions & 23 deletions pkg/ccl/backupccl/backup_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1226,19 +1226,16 @@ func TestEncryptedBackupRestoreSystemJobs(t *testing.T) {
} {
t.Run(tc.name, func(t *testing.T) {
var encryptionOption string
var sanitizedEncryptionOption1 string
var sanitizedEncryptionOption2 string
var sanitizedEncryptionOption string
if tc.useKMS {
correctKMSURI, _ := getAWSKMSURI(t, regionEnvVariable, keyIDEnvVariable)
encryptionOption = fmt.Sprintf("kms = '%s'", correctKMSURI)
sanitizedURI, err := redactTestKMSURI(correctKMSURI)
require.NoError(t, err)
sanitizedEncryptionOption1 = fmt.Sprintf("kms = '%s'", sanitizedURI)
sanitizedEncryptionOption2 = sanitizedEncryptionOption1
sanitizedEncryptionOption = fmt.Sprintf("kms = '%s'", sanitizedURI)
} else {
encryptionOption = "encryption_passphrase = 'abcdefg'"
sanitizedEncryptionOption1 = "encryption_passphrase = '*****'"
sanitizedEncryptionOption2 = "encryption_passphrase = 'redacted'"
sanitizedEncryptionOption = "encryption_passphrase = '*****'"
}
_, sqlDB, _, cleanupFn := backupRestoreTestSetup(t, multiNode, 3, InitManualReplication)
conn := sqlDB.DB.(*gosql.DB)
Expand All @@ -1261,7 +1258,7 @@ func TestEncryptedBackupRestoreSystemJobs(t *testing.T) {
Username: username.RootUserName(),
Description: fmt.Sprintf(
`BACKUP DATABASE data TO '%s' WITH %s`,
backupLoc1, sanitizedEncryptionOption1),
backupLoc1, sanitizedEncryptionOption),
DescriptorIDs: descpb.IDs{
descpb.ID(backupDatabaseID),
descpb.ID(backupSchemaID),
Expand All @@ -1280,7 +1277,7 @@ into_db='restoredb', %s)`, encryptionOption), backupLoc1)
Username: username.RootUserName(),
Description: fmt.Sprintf(
`RESTORE TABLE data.bank FROM '%s' WITH %s, into_db = 'restoredb'`,
backupLoc1, sanitizedEncryptionOption2,
backupLoc1, sanitizedEncryptionOption,
),
DescriptorIDs: descpb.IDs{
descpb.ID(restoreDatabaseID + 1),
Expand Down Expand Up @@ -4256,9 +4253,8 @@ func TestEncryptedBackup(t *testing.T) {

sqlDB.Exec(t, fmt.Sprintf(`SHOW BACKUP $1 WITH %s`, encryptionOption), backupLoc1)

sqlDB.Exec(t, fmt.Sprintf(`SHOW BACKUP $1 WITH %s,encryption_info_dir='%s'`,
encryptionOption,
backupLoc1),
sqlDB.Exec(t, fmt.Sprintf(`SHOW BACKUP $1 WITH %s, encryption_info_dir = '%s'`,
encryptionOption, backupLoc1),
backupLoc1inc)

var expectedShowError string
Expand Down Expand Up @@ -4330,7 +4326,7 @@ func TestRegionalKMSEncryptedBackup(t *testing.T) {
backupLoc1 := localFoo + "/x?COCKROACH_LOCALITY=default"
backupLoc2 := localFoo + "/x2?COCKROACH_LOCALITY=" + url.QueryEscape("dc=dc1")

sqlDB.Exec(t, fmt.Sprintf(`BACKUP TO ($1, $2) WITH %s`,
sqlDB.Exec(t, fmt.Sprintf(`BACKUP INTO ($1, $2) WITH %s`,
concatMultiRegionKMSURIs(multiRegionKMSURIs)), backupLoc1,
backupLoc2)

Expand All @@ -4342,14 +4338,18 @@ func TestRegionalKMSEncryptedBackup(t *testing.T) {

checkBackupFilesEncrypted(t, rawDir)

sqlDB.Exec(t, fmt.Sprintf(`SHOW BACKUP $1 WITH KMS='%s'`, multiRegionKMSURIs[0]),
// check that SHOW BACKUP is valid when a single kms uri is provided and when multiple are
sqlDB.Exec(t, fmt.Sprintf(`SHOW BACKUP FROM LATEST IN $1 WITH KMS='%s'`, multiRegionKMSURIs[0]),
backupLoc1)

sqlDB.Exec(t, fmt.Sprintf(`SHOW BACKUP FROM LATEST IN ($1, $2) WITH %s`,
concatMultiRegionKMSURIs(multiRegionKMSURIs)), backupLoc1, backupLoc2)

// Attempt to RESTORE using each of the regional KMSs independently.
for _, uri := range multiRegionKMSURIs {
sqlDB.Exec(t, `DROP DATABASE neverappears CASCADE`)

sqlDB.Exec(t, fmt.Sprintf(`RESTORE DATABASE neverappears FROM ($1, $2) WITH %s`,
sqlDB.Exec(t, fmt.Sprintf(`RESTORE DATABASE neverappears FROM LATEST IN ($1, $2) WITH %s`,
concatMultiRegionKMSURIs([]string{uri})), backupLoc1, backupLoc2)

sqlDB.CheckQueryResults(t, `SHOW EXPERIMENTAL_FINGERPRINTS FROM TABLE neverappears.neverappears`, before)
Expand Down Expand Up @@ -9899,18 +9899,13 @@ func TestBackupRestoreSeparateExplicitIsDefault(t *testing.T) {
sqlDB.Exec(t, fmt.Sprintf("SHOW BACKUPS IN %s", dest))
sqlDB.Exec(t, fmt.Sprintf("SHOW BACKUP FROM LATEST IN %s", dest))

// Locality aware show backups will eventually fail if not all localities are provided,
// but for now, they're ok.
if len(br.dest) > 1 {
// Locality aware show backups will eventually fail if not all localities are provided,
// but for now, they're ok.
sqlDB.Exec(t, fmt.Sprintf("SHOW BACKUP FROM LATEST IN %s", br.dest[1]))

errorMsg := "SHOW BACKUP on locality aware backups using incremental_location is not" +
" supported yet"
sqlDB.ExpectErr(t, errorMsg, fmt.Sprintf("SHOW BACKUP FROM LATEST IN %s WITH incremental_location= %s", dest, br.inc[0]))
} else {
// non locality aware show backup with incremental_location should work!
sqlDB.Exec(t, fmt.Sprintf("SHOW BACKUP FROM LATEST IN %s WITH incremental_location= %s", dest, inc))
}

sqlDB.Exec(t, fmt.Sprintf("SHOW BACKUP FROM LATEST IN %s WITH incremental_location= %s", dest, inc))
}
sir := fmt.Sprintf("RESTORE DATABASE fkdb FROM LATEST IN %s WITH new_db_name = 'inc_fkdb', incremental_location = %s", dest, inc)
sqlDB.Exec(t, sir)
Expand Down
Loading

0 comments on commit 5f0c56e

Please sign in to comment.