Merge cockroachdb#110408 cockroachdb#110580

110408: pgcryptocipherccl: don't panic when passed invalid padding size r=rafiss a=andyyang890 This patch modifies `zeroPadOrTruncate` to return an error instead of panicking when it is passed an invalid (i.e. negative) padding size. Note that this is currently only possible as the result of programmer error. We also add a unit test to validate the function's expected behavior. Informs cockroachdb#21001 Release note: None 110580: tree: fix formatting of SHOW BACKUP WITH OPTIONS r=rafiss a=rafiss This avoids an ambiguity when formatting the statement. fixes cockroachdb#110411 Release note: None Co-authored-by: Andy Yang <[email protected]> Co-authored-by: Rafi Shamim <[email protected]>
jbowens · Sep 15, 2023 · 28552c8 · 28552c8
3 parents 556be34 + 7addf4f + e1fbdfe
commit 28552c8
Show file tree

Hide file tree

Showing 9 changed files with 160 additions and 77 deletions.
diff --git a/docs/generated/sql/bnf/show_backup.bnf b/docs/generated/sql/bnf/show_backup.bnf
@@ -7,5 +7,4 @@ show_backup_stmt ::=
 	| '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' 'BACKUP' 'CONNECTION' string_or_placeholder
-	| 'SHOW' 'BACKUP' 'CONNECTION' string_or_placeholder 'WITH' show_backup_connection_options_list
+	| 'SHOW' 'BACKUP' 'CONNECTION' string_or_placeholder opt_with_show_backup_connection_options_list
diff --git a/docs/generated/sql/bnf/stmt_block.bnf b/docs/generated/sql/bnf/stmt_block.bnf
@@ -796,8 +796,7 @@ show_backup_stmt ::=
 	| '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' 'BACKUP' 'CONNECTION' string_or_placeholder
-	| 'SHOW' 'BACKUP' 'CONNECTION' string_or_placeholder 'WITH' show_backup_connection_options_list
+	| 'SHOW' 'BACKUP' 'CONNECTION' string_or_placeholder opt_with_show_backup_connection_options_list
 
 show_columns_stmt ::=
 	'SHOW' 'COLUMNS' 'FROM' table_name with_comment
@@ -1946,8 +1945,10 @@ opt_with_show_backup_options ::=
 	| 'WITH' 'OPTIONS' '(' show_backup_options_list ')'
 	| 
 
-show_backup_connection_options_list ::=
-	( show_backup_connection_options ) ( ( ',' show_backup_connection_options ) )*
+opt_with_show_backup_connection_options_list ::=
+	'WITH' show_backup_connection_options_list
+	| 'WITH' 'OPTIONS' '(' show_backup_connection_options_list ')'
+	| 
 
 with_comment ::=
 	'WITH' 'COMMENT'
@@ -2696,10 +2697,8 @@ extra_var_value ::=
 show_backup_options_list ::=
 	( show_backup_options ) ( ( ',' show_backup_options ) )*
 
-show_backup_connection_options ::=
-	'TRANSFER' '=' string_or_placeholder
-	| 'TIME' '=' string_or_placeholder
-	| 'CONCURRENTLY' '=' a_expr
+show_backup_connection_options_list ::=
+	( show_backup_connection_options ) ( ( ',' show_backup_connection_options ) )*
 
 targets_roles ::=
 	'ROLE' role_spec_list
@@ -3262,6 +3261,11 @@ show_backup_options ::=
 	| 'ENCRYPTION_INFO_DIR' '=' string_or_placeholder
 	| 'DEBUG_DUMP_METADATA_SST'
 
+show_backup_connection_options ::=
+	'TRANSFER' '=' string_or_placeholder
+	| 'TIME' '=' string_or_placeholder
+	| 'CONCURRENTLY' '=' a_expr
+
 schema_wildcard ::=
 	wildcard_pattern
 

diff --git a/pkg/ccl/pgcryptoccl/pgcryptocipherccl/cipher.go b/pkg/ccl/pgcryptoccl/pgcryptocipherccl/cipher.go
@@ -74,13 +74,17 @@ func Decrypt(data []byte, key []byte, iv []byte, cipherType string) ([]byte, err
 func newCipher(method cipherMethod, key []byte) (cipher.Block, error) {
 	switch a := method.algorithm; a {
 	case aesCipher:
+		var err error
 		switch l := len(key); {
 		case l >= 32:
-			key = zeroPadOrTruncate(key, 32)
+			key, err = zeroPadOrTruncate(key, 32)
 		case l >= 24:
-			key = zeroPadOrTruncate(key, 24)
+			key, err = zeroPadOrTruncate(key, 24)
 		default:
-			key = zeroPadOrTruncate(key, 16)
+			key, err = zeroPadOrTruncate(key, 16)
+		}
+		if err != nil {
+			return nil, err
 		}
 		return aes.NewCipher(key)
 	default:
@@ -124,8 +128,12 @@ func validateDataLength(data []byte, blockSize int) error {
 func encrypt(method cipherMethod, block cipher.Block, iv []byte, data []byte) ([]byte, error) {
 	switch m := method.mode; m {
 	case cbcMode:
+		var err error
 		ret := make([]byte, len(data))
-		iv = zeroPadOrTruncate(iv, block.BlockSize())
+		iv, err = zeroPadOrTruncate(iv, block.BlockSize())
+		if err != nil {
+			return nil, err
+		}
 		mode := cipher.NewCBCEncrypter(block, iv)
 		mode.CryptBlocks(ret, data)
 		return ret, nil
@@ -137,21 +145,16 @@ func encrypt(method cipherMethod, block cipher.Block, iv []byte, data []byte) ([
 func decrypt(method cipherMethod, block cipher.Block, iv []byte, data []byte) ([]byte, error) {
 	switch m := method.mode; m {
 	case cbcMode:
+		var err error
 		ret := make([]byte, len(data))
-		iv = zeroPadOrTruncate(iv, block.BlockSize())
+		iv, err = zeroPadOrTruncate(iv, block.BlockSize())
+		if err != nil {
+			return nil, err
+		}
 		mode := cipher.NewCBCDecrypter(block, iv)
 		mode.CryptBlocks(ret, data)
 		return ret, nil
 	default:
 		return nil, errors.Newf("cannot encrypt for unknown mode: %d", m)
 	}
 }
-
-func zeroPadOrTruncate(data []byte, size int) []byte {
-	if len(data) >= size {
-		return data[:size]
-	}
-	paddedData := make([]byte, size)
-	copy(paddedData, data)
-	return paddedData
-}
diff --git a/pkg/ccl/pgcryptoccl/pgcryptocipherccl/padding.go b/pkg/ccl/pgcryptoccl/pgcryptocipherccl/padding.go
@@ -51,3 +51,17 @@ func pkcsUnpad(data []byte) ([]byte, error) {
 
 	return data[:len(data)-int(paddingLen)], nil
 }
+
+// zeroPadOrTruncate pads a slice of bytes with zeroes if its length is smaller
+// than size and truncates the slice to length size otherwise.
+func zeroPadOrTruncate(data []byte, size int) ([]byte, error) {
+	if size < 0 {
+		return nil, errors.AssertionFailedf("cannot zero pad or truncate to negative size")
+	}
+	if len(data) >= size {
+		return data[:size], nil
+	}
+	paddedData := make([]byte, size)
+	copy(paddedData, data)
+	return paddedData, nil
+}
diff --git a/pkg/ccl/pgcryptoccl/pgcryptocipherccl/padding_test.go b/pkg/ccl/pgcryptoccl/pgcryptocipherccl/padding_test.go
@@ -110,3 +110,49 @@ func TestPKCSUnpad(t *testing.T) {
 		})
 	}
 }
+
+func TestZeroPadOrTruncate(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+
+	for name, tc := range map[string]struct {
+		data        []byte
+		size        int
+		expected    []byte
+		expectedErr string
+	}{
+		"data length less than size": {
+			data:     []byte{1, 2},
+			size:     3,
+			expected: []byte{1, 2, 0},
+		},
+		"data length equal to size": {
+			data:     []byte{1, 2, 3},
+			size:     3,
+			expected: []byte{1, 2, 3},
+		},
+		"data length greater than size": {
+			data:     []byte{1, 2, 3, 4},
+			size:     3,
+			expected: []byte{1, 2, 3},
+		},
+		"empty data": {
+			data:     nil,
+			size:     3,
+			expected: []byte{0, 0, 0},
+		},
+		"negative size": {
+			data:        []byte{1, 2, 3},
+			size:        -1,
+			expectedErr: "cannot zero pad or truncate to negative size",
+		},
+	} {
+		t.Run(name, func(t *testing.T) {
+			actual, err := zeroPadOrTruncate(tc.data, tc.size)
+			if tc.expectedErr != "" {
+				require.EqualError(t, err, tc.expectedErr)
+				return
+			}
+			require.Equal(t, tc.expected, actual)
+		})
+	}
+}
diff --git a/pkg/sql/parser/sql.y b/pkg/sql/parser/sql.y
@@ -1347,7 +1347,7 @@ func (u *sqlSymUnion) beginTransaction() *tree.BeginTransaction {
 %type <*tree.TenantReplicationOptions> opt_with_replication_options replication_options replication_options_list
 %type <tree.ShowBackupDetails> show_backup_details
 %type <*tree.ShowJobOptions> show_job_options show_job_options_list
-%type <*tree.ShowBackupOptions> opt_with_show_backup_options show_backup_options show_backup_options_list show_backup_connection_options show_backup_connection_options_list
+%type <*tree.ShowBackupOptions> opt_with_show_backup_options show_backup_options show_backup_options_list show_backup_connection_options opt_with_show_backup_connection_options_list show_backup_connection_options_list
 %type <*tree.CopyOptions> opt_with_copy_options copy_options copy_options_list copy_generic_options copy_generic_options_list
 %type <str> import_format
 %type <str> storage_parameter_key
@@ -7611,19 +7611,12 @@ show_backup_stmt:
   			Options: *$5.showBackupOptions(),
   		}
   	}
-| SHOW BACKUP CONNECTION string_or_placeholder
+| SHOW BACKUP CONNECTION string_or_placeholder opt_with_show_backup_connection_options_list
   	{
   		$$.val = &tree.ShowBackup{
   		  Details:  tree.BackupConnectionTest,
   			Path:    $4.expr(),
-  		}
-  	}
-| SHOW BACKUP CONNECTION string_or_placeholder WITH show_backup_connection_options_list
-  	{
-  		$$.val = &tree.ShowBackup{
-  		  Details:  tree.BackupConnectionTest,
-  			Path:    $4.expr(),
-        Options: *$6.showBackupOptions(),
+  			Options: *$5.showBackupOptions(),
   		}
   	}
 | SHOW BACKUP error // SHOW HELP: SHOW BACKUP
@@ -7724,6 +7717,20 @@ show_backup_options:
  $$.val = &tree.ShowBackupOptions{DebugMetadataSST: true}
  }
 
+opt_with_show_backup_connection_options_list:
+  WITH show_backup_connection_options_list
+  {
+    $$.val = $2.showBackupOptions()
+  }
+| WITH OPTIONS '(' show_backup_connection_options_list ')'
+  {
+    $$.val = $4.showBackupOptions()
+  }
+| /* EMPTY */
+  {
+    $$.val = &tree.ShowBackupOptions{}
+  }
+
 show_backup_connection_options_list:
   // Require at least one option
   show_backup_connection_options

diff --git a/pkg/sql/parser/testdata/backup_restore b/pkg/sql/parser/testdata/backup_restore
@@ -105,27 +105,27 @@ SHOW BACKUP 'bar' -- identifiers removed
 parse
 SHOW BACKUP 'bar' WITH ENCRYPTION_PASSPHRASE = 'secret', CHECK_FILES
 ----
-SHOW BACKUP 'bar' WITH check_files, encryption_passphrase = '*****' -- normalized!
-SHOW BACKUP ('bar') WITH check_files, encryption_passphrase = '*****' -- fully parenthesized
-SHOW BACKUP '_' WITH check_files, encryption_passphrase = '*****' -- literals removed
-SHOW BACKUP 'bar' WITH check_files, encryption_passphrase = '*****' -- identifiers removed
-SHOW BACKUP 'bar' WITH check_files, encryption_passphrase = 'secret' -- passwords exposed
+SHOW BACKUP 'bar' WITH OPTIONS (check_files, encryption_passphrase = '*****') -- normalized!
+SHOW BACKUP ('bar') WITH OPTIONS (check_files, encryption_passphrase = '*****') -- fully parenthesized
+SHOW BACKUP '_' WITH OPTIONS (check_files, encryption_passphrase = '*****') -- literals removed
+SHOW BACKUP 'bar' WITH OPTIONS (check_files, encryption_passphrase = '*****') -- identifiers removed
+SHOW BACKUP 'bar' WITH OPTIONS (check_files, encryption_passphrase = 'secret') -- passwords exposed
 
 parse
 SHOW BACKUP FROM LATEST IN 'bar' WITH incremental_location = 'baz', skip size
 ----
-SHOW BACKUP FROM 'latest' IN 'bar' WITH incremental_location = 'baz', skip size -- normalized!
-SHOW BACKUP FROM ('latest') IN ('bar') WITH incremental_location = ('baz'), skip size -- fully parenthesized
-SHOW BACKUP FROM '_' IN '_' WITH incremental_location = '_', skip size -- literals removed
-SHOW BACKUP FROM 'latest' IN 'bar' WITH incremental_location = 'baz', skip size -- identifiers removed
+SHOW BACKUP FROM 'latest' IN 'bar' WITH OPTIONS (incremental_location = 'baz', skip size) -- normalized!
+SHOW BACKUP FROM ('latest') IN ('bar') WITH OPTIONS (incremental_location = ('baz'), skip size) -- fully parenthesized
+SHOW BACKUP FROM '_' IN '_' WITH OPTIONS (incremental_location = '_', skip size) -- literals removed
+SHOW BACKUP FROM 'latest' IN 'bar' WITH OPTIONS (incremental_location = 'baz', skip size) -- identifiers removed
 
 parse
 SHOW BACKUP FROM LATEST IN ('bar','bar1') WITH KMS = ('foo', 'bar'), incremental_location=('hi','hello')
 ----
-SHOW BACKUP FROM 'latest' IN ('bar', 'bar1') WITH incremental_location = ('hi', 'hello'), kms = ('foo', 'bar') -- normalized!
-SHOW BACKUP FROM ('latest') IN (('bar'), ('bar1')) WITH incremental_location = (('hi'), ('hello')), kms = (('foo'), ('bar')) -- fully parenthesized
-SHOW BACKUP FROM '_' IN ('_', '_') WITH incremental_location = ('_', '_'), kms = ('_', '_') -- literals removed
-SHOW BACKUP FROM 'latest' IN ('bar', 'bar1') WITH incremental_location = ('hi', 'hello'), kms = ('foo', 'bar') -- identifiers removed
+SHOW BACKUP FROM 'latest' IN ('bar', 'bar1') WITH OPTIONS (incremental_location = ('hi', 'hello'), kms = ('foo', 'bar')) -- normalized!
+SHOW BACKUP FROM ('latest') IN (('bar'), ('bar1')) WITH OPTIONS (incremental_location = (('hi'), ('hello')), kms = (('foo'), ('bar'))) -- fully parenthesized
+SHOW BACKUP FROM '_' IN ('_', '_') WITH OPTIONS (incremental_location = ('_', '_'), kms = ('_', '_')) -- literals removed
+SHOW BACKUP FROM 'latest' IN ('bar', 'bar1') WITH OPTIONS (incremental_location = ('hi', 'hello'), kms = ('foo', 'bar')) -- identifiers removed
 
 
 parse
@@ -163,18 +163,18 @@ SHOW BACKUP CONNECTION 'bar' -- identifiers removed
 parse
 SHOW BACKUP CONNECTION 'bar' WITH TRANSFER = '1KiB', TIME = '1h', CONCURRENTLY = 3
 ----
-SHOW BACKUP CONNECTION 'bar' WITH CONCURRENTLY = 3, TRANSFER = '1KiB', TIME = '1h' -- normalized!
-SHOW BACKUP CONNECTION ('bar') WITH CONCURRENTLY = (3), TRANSFER = ('1KiB'), TIME = ('1h') -- fully parenthesized
-SHOW BACKUP CONNECTION '_' WITH CONCURRENTLY = _, TRANSFER = '_', TIME = '_' -- literals removed
-SHOW BACKUP CONNECTION 'bar' WITH CONCURRENTLY = 3, TRANSFER = '1KiB', TIME = '1h' -- identifiers removed
+SHOW BACKUP CONNECTION 'bar' WITH OPTIONS (CONCURRENTLY = 3, TRANSFER = '1KiB', TIME = '1h') -- normalized!
+SHOW BACKUP CONNECTION ('bar') WITH OPTIONS (CONCURRENTLY = (3), TRANSFER = ('1KiB'), TIME = ('1h')) -- fully parenthesized
+SHOW BACKUP CONNECTION '_' WITH OPTIONS (CONCURRENTLY = _, TRANSFER = '_', TIME = '_') -- literals removed
+SHOW BACKUP CONNECTION 'bar' WITH OPTIONS (CONCURRENTLY = 3, TRANSFER = '1KiB', TIME = '1h') -- identifiers removed
 
 parse
 SHOW BACKUP CONNECTION 'bar' WITH TRANSFER = $1, CONCURRENTLY = $2, TIME = $3
 ----
-SHOW BACKUP CONNECTION 'bar' WITH CONCURRENTLY = $2, TRANSFER = $1, TIME = $3 -- normalized!
-SHOW BACKUP CONNECTION ('bar') WITH CONCURRENTLY = ($2), TRANSFER = ($1), TIME = ($3) -- fully parenthesized
-SHOW BACKUP CONNECTION '_' WITH CONCURRENTLY = $1, TRANSFER = $1, TIME = $1 -- literals removed
-SHOW BACKUP CONNECTION 'bar' WITH CONCURRENTLY = $2, TRANSFER = $1, TIME = $3 -- identifiers removed
+SHOW BACKUP CONNECTION 'bar' WITH OPTIONS (CONCURRENTLY = $2, TRANSFER = $1, TIME = $3) -- normalized!
+SHOW BACKUP CONNECTION ('bar') WITH OPTIONS (CONCURRENTLY = ($2), TRANSFER = ($1), TIME = ($3)) -- fully parenthesized
+SHOW BACKUP CONNECTION '_' WITH OPTIONS (CONCURRENTLY = $1, TRANSFER = $1, TIME = $1) -- literals removed
+SHOW BACKUP CONNECTION 'bar' WITH OPTIONS (CONCURRENTLY = $2, TRANSFER = $1, TIME = $3) -- identifiers removed
 
 parse
 SHOW BACKUPS IN 'bar'
@@ -203,10 +203,10 @@ SHOW BACKUP 'foo' IN 'bar' -- identifiers removed
 parse
 SHOW BACKUP FROM $1 IN $2 WITH privileges
 ----
-SHOW BACKUP FROM $1 IN $2 WITH privileges
-SHOW BACKUP FROM ($1) IN ($2) WITH privileges -- fully parenthesized
-SHOW BACKUP FROM $1 IN $1 WITH privileges -- literals removed
-SHOW BACKUP FROM $1 IN $2 WITH privileges -- identifiers removed
+SHOW BACKUP FROM $1 IN $2 WITH OPTIONS (privileges) -- normalized!
+SHOW BACKUP FROM ($1) IN ($2) WITH OPTIONS (privileges) -- fully parenthesized
+SHOW BACKUP FROM $1 IN $1 WITH OPTIONS (privileges) -- literals removed
+SHOW BACKUP FROM $1 IN $2 WITH OPTIONS (privileges) -- identifiers removed
 
 parse
 SHOW BACKUP FILES FROM 'foo' IN 'bar'
@@ -235,11 +235,11 @@ SHOW BACKUP SCHEMAS FROM 'foo' IN 'bar' -- identifiers removed
 parse
 SHOW BACKUP $1 IN $2 WITH ENCRYPTION_PASSPHRASE = 'secret', ENCRYPTION_INFO_DIR = 'long_live_backupper'
 ----
-SHOW BACKUP $1 IN $2 WITH encryption_passphrase = '*****', encryption_info_dir = 'long_live_backupper' -- normalized!
-SHOW BACKUP ($1) IN ($2) WITH encryption_passphrase = '*****', encryption_info_dir = ('long_live_backupper') -- fully parenthesized
-SHOW BACKUP $1 IN $1 WITH encryption_passphrase = '*****', encryption_info_dir = '_' -- literals removed
-SHOW BACKUP $1 IN $2 WITH encryption_passphrase = '*****', encryption_info_dir = 'long_live_backupper' -- identifiers removed
-SHOW BACKUP $1 IN $2 WITH encryption_passphrase = 'secret', encryption_info_dir = 'long_live_backupper' -- passwords exposed
+SHOW BACKUP $1 IN $2 WITH OPTIONS (encryption_passphrase = '*****', encryption_info_dir = 'long_live_backupper') -- normalized!
+SHOW BACKUP ($1) IN ($2) WITH OPTIONS (encryption_passphrase = '*****', encryption_info_dir = ('long_live_backupper')) -- fully parenthesized
+SHOW BACKUP $1 IN $1 WITH OPTIONS (encryption_passphrase = '*****', encryption_info_dir = '_') -- literals removed
+SHOW BACKUP $1 IN $2 WITH OPTIONS (encryption_passphrase = '*****', encryption_info_dir = 'long_live_backupper') -- identifiers removed
+SHOW BACKUP $1 IN $2 WITH OPTIONS (encryption_passphrase = 'secret', encryption_info_dir = 'long_live_backupper') -- passwords exposed
 
 parse
 BACKUP TABLE foo TO 'bar' AS OF SYSTEM TIME '1' INCREMENTAL FROM 'baz'
@@ -1028,3 +1028,12 @@ BACKUP INTO LATEST IN 'unlogged' -- normalized!
 BACKUP INTO LATEST IN ('unlogged') -- fully parenthesized
 BACKUP INTO LATEST IN '_' -- literals removed
 BACKUP INTO LATEST IN 'unlogged' -- identifiers removed
+
+# Regression test for https://github.com/cockroachdb/cockroach/issues/110411.
+parse
+SHOW BACKUP CONNECTION 'bar' WITH OPTIONS (TIME = '1h')
+----
+SHOW BACKUP CONNECTION 'bar' WITH OPTIONS (TIME = '1h')
+SHOW BACKUP CONNECTION ('bar') WITH OPTIONS (TIME = ('1h')) -- fully parenthesized
+SHOW BACKUP CONNECTION '_' WITH OPTIONS (TIME = '_') -- literals removed
+SHOW BACKUP CONNECTION 'bar' WITH OPTIONS (TIME = '1h') -- identifiers removed
diff --git a/pkg/sql/parser/testdata/show b/pkg/sql/parser/testdata/show
@@ -2055,23 +2055,23 @@ SHOW VIRTUAL CLUSTER _ WITH REPLICATION STATUS, CAPABILITIES -- identifiers remo
 parse
 SHOW BACKUP 'family' IN ('string', 'placeholder', 'placeholder', 'placeholder', 'string', 'placeholder', 'string', 'placeholder') WITH incremental_location = 'nullif', privileges, debug_dump_metadata_sst
 ----
-SHOW BACKUP 'family' IN ('string', 'placeholder', 'placeholder', 'placeholder', 'string', 'placeholder', 'string', 'placeholder') WITH incremental_location = 'nullif', privileges, debug_dump_metadata_sst
-SHOW BACKUP ('family') IN (('string'), ('placeholder'), ('placeholder'), ('placeholder'), ('string'), ('placeholder'), ('string'), ('placeholder')) WITH incremental_location = ('nullif'), privileges, debug_dump_metadata_sst -- fully parenthesized
-SHOW BACKUP '_' IN ('_', '_', '_', '_', '_', '_', '_', '_') WITH incremental_location = '_', privileges, debug_dump_metadata_sst -- literals removed
-SHOW BACKUP 'family' IN ('string', 'placeholder', 'placeholder', 'placeholder', 'string', 'placeholder', 'string', 'placeholder') WITH incremental_location = 'nullif', privileges, debug_dump_metadata_sst -- identifiers removed
+SHOW BACKUP 'family' IN ('string', 'placeholder', 'placeholder', 'placeholder', 'string', 'placeholder', 'string', 'placeholder') WITH OPTIONS (incremental_location = 'nullif', privileges, debug_dump_metadata_sst) -- normalized!
+SHOW BACKUP ('family') IN (('string'), ('placeholder'), ('placeholder'), ('placeholder'), ('string'), ('placeholder'), ('string'), ('placeholder')) WITH OPTIONS (incremental_location = ('nullif'), privileges, debug_dump_metadata_sst) -- fully parenthesized
+SHOW BACKUP '_' IN ('_', '_', '_', '_', '_', '_', '_', '_') WITH OPTIONS (incremental_location = '_', privileges, debug_dump_metadata_sst) -- literals removed
+SHOW BACKUP 'family' IN ('string', 'placeholder', 'placeholder', 'placeholder', 'string', 'placeholder', 'string', 'placeholder') WITH OPTIONS (incremental_location = 'nullif', privileges, debug_dump_metadata_sst) -- identifiers removed
 
 parse
 SHOW BACKUP 'abc' WITH SKIP SIZE
 ----
-SHOW BACKUP 'abc' WITH skip size -- normalized!
-SHOW BACKUP ('abc') WITH skip size -- fully parenthesized
-SHOW BACKUP '_' WITH skip size -- literals removed
-SHOW BACKUP 'abc' WITH skip size -- identifiers removed
+SHOW BACKUP 'abc' WITH OPTIONS (skip size) -- normalized!
+SHOW BACKUP ('abc') WITH OPTIONS (skip size) -- fully parenthesized
+SHOW BACKUP '_' WITH OPTIONS (skip size) -- literals removed
+SHOW BACKUP 'abc' WITH OPTIONS (skip size) -- identifiers removed
 
 parse
 SHOW BACKUP 'abc' WITH NOWAIT
 ----
-SHOW BACKUP 'abc' WITH skip size -- normalized!
-SHOW BACKUP ('abc') WITH skip size -- fully parenthesized
-SHOW BACKUP '_' WITH skip size -- literals removed
-SHOW BACKUP 'abc' WITH skip size -- identifiers removed
+SHOW BACKUP 'abc' WITH OPTIONS (skip size) -- normalized!
+SHOW BACKUP ('abc') WITH OPTIONS (skip size) -- fully parenthesized
+SHOW BACKUP '_' WITH OPTIONS (skip size) -- literals removed
+SHOW BACKUP 'abc' WITH OPTIONS (skip size) -- identifiers removed
diff --git a/pkg/sql/sem/tree/show.go b/pkg/sql/sem/tree/show.go
@@ -139,8 +139,9 @@ func (node *ShowBackup) Format(ctx *FmtCtx) {
 		ctx.FormatNode(&node.InCollection)
 	}
 	if !node.Options.IsDefault() {
-		ctx.WriteString(" WITH ")
+		ctx.WriteString(" WITH OPTIONS (")
 		ctx.FormatNode(&node.Options)
+		ctx.WriteString(")")
 	}
 }