backupccl: explicitly parse SHOW BACKUP options #95562

msbutler · 2023-01-19T23:03:46Z

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:

A user can now pass and check multiple KMS URIs in
SHOW BACKUP
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

blathers-crl · 2023-01-19T23:03:49Z

It looks like your PR touches SQL parser code but doesn't add or edit parser tests. Please make sure you add or edit parser tests if you edit the parser.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

cockroach-teamcity · 2023-01-19T23:03:55Z

This change is

benbardin · 2023-01-23T17:44:35Z

Oof. I see where this is coming from but adding so many keywords feels off, since this is a sub-option for one query. Why isn't this an issue for the other commands?

msbutler · 2023-01-23T17:57:17Z

Why isn't this an issue for the other commands?

Like other cockroach sql cmds with an extensive Options list? I'm not sure I follow. I think what I propose is the conventional way to do it? E.g. backup, restore, copy

benbardin

Discussed offline: Michael showed me that the base BACKUP and RESTORE commands already use this query structure, e.g. with a bunch of keywords. By adding the keywords here, we're bringing the SHOW BACKUP command to parity with these (both in query data structure, and in user functionality)

pkg/sql/sem/tree/show.go

stevendanna

I think my main question wrt all of the new keywords is whether we definitely want to promote some of these options into documented, supported options.

stevendanna · 2023-01-26T14:36:49Z

pkg/sql/parser/sql.y

@@ -15810,6 +15883,7 @@ unreserved_keyword:
 | ENCODING
 | ENCRYPTED
 | ENCRYPTION_PASSPHRASE
+| ENCRYPTION_INFO_DIR


Please add these to the bare_label_keyword list as well.

also, you mentioned in some meeting today that you'd file/ or work on a stability period issue that would create a linter for syntax changes right? I imagine we also need to add a bunch of backup/restore keywords to this list as well.

stevendanna · 2023-01-26T14:38:45Z

pkg/sql/sem/tree/show.go

+	IncrementalStorage   StringOrPlaceholderOptList
+	DecryptionKMSURI     StringOrPlaceholderOptList
+	EncryptionPassphrase Expr


Just noting: We don't currently have an implementation of walkableStmt for ShowBackup. If we did, I would say we should probably be updating it to account for these new Exprs. We may want to add one in a follow up.

Sounds good. Just added a todo. I don't know enough about this method to understand why one would need it. It seems that no other SHOW cmd in this file implements one either.

msbutler

@stevendanna on your question on keyword documentation: I tried to describe what needed to be documented in the commit message. Let me know if that satisfied you!

msbutler · 2023-01-26T18:36:30Z

pkg/sql/parser/sql.y

@@ -15810,6 +15883,7 @@ unreserved_keyword:
 | ENCODING
 | ENCRYPTED
 | ENCRYPTION_PASSPHRASE
+| ENCRYPTION_INFO_DIR


msbutler · 2023-01-26T18:39:41Z

pkg/sql/sem/tree/show.go

+	IncrementalStorage   StringOrPlaceholderOptList
+	DecryptionKMSURI     StringOrPlaceholderOptList
+	EncryptionPassphrase Expr


Sounds good. Just added a todo. I don't know enough about this method to understand why one would need it. It seems that no other SHOW cmd in this file implements one either.

msbutler · 2023-01-26T19:35:43Z

unrelated extended CI failure

msbutler · 2023-01-26T21:58:11Z

unrelated CI flake

stevendanna · 2023-01-27T12:52:48Z

I tried to describe what needed to be documented in the commit message.

Heh, I read that, thought it was good, and then forgot it by the time I finished my review. Sorry about that! LGTM

Release note (security update): previously, the ENCRYPTION_PASSPHRASE option passed to RESTORE would appear as 'redacted', and now it appears as '******' which is consistent with SHOW BACKUP and BACKUP. Epic: None

Fixes: cockroachdb#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

msbutler · 2023-01-27T14:31:28Z

bors r=benbardin

craig · 2023-01-27T16:45:28Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2023-01-27T18:40:56Z

Build succeeded:

Bazel Essential CI (Cockroach)

msbutler force-pushed the butler-show-inc branch from 9a42507 to bd7412d Compare January 19, 2023 23:23

msbutler self-assigned this Jan 19, 2023

msbutler force-pushed the butler-show-inc branch 5 times, most recently from 362b07f to 4d665b9 Compare January 23, 2023 01:17

msbutler changed the title ~~Butler show inc~~ backupccl: explicitly parse SHOW BACKUP options Jan 23, 2023

msbutler marked this pull request as ready for review January 23, 2023 01:18

msbutler requested review from a team as code owners January 23, 2023 01:18

msbutler requested review from rhu713, benbardin and dt and removed request for a team January 23, 2023 01:18

msbutler added the T-disaster-recovery label Jan 23, 2023

benbardin reviewed Jan 23, 2023

View reviewed changes

pkg/sql/sem/tree/show.go Outdated Show resolved Hide resolved

pkg/sql/sem/tree/show.go Show resolved Hide resolved

msbutler commented Jan 23, 2023

View reviewed changes

pkg/sql/sem/tree/show.go Show resolved Hide resolved

pkg/sql/sem/tree/show.go Show resolved Hide resolved

stevendanna reviewed Jan 26, 2023

View reviewed changes

benbardin approved these changes Jan 26, 2023

View reviewed changes

msbutler force-pushed the butler-show-inc branch 2 times, most recently from 4379e90 to dea9031 Compare January 26, 2023 18:43

msbutler commented Jan 26, 2023

View reviewed changes

msbutler force-pushed the butler-show-inc branch from dea9031 to fef1dde Compare January 26, 2023 19:49

msbutler added 2 commits January 27, 2023 08:25

backupccl: sanitize ENCRYPTION_PASSPHRASE option in RESTORE in parser

6af080b

Release note (security update): previously, the ENCRYPTION_PASSPHRASE option passed to RESTORE would appear as 'redacted', and now it appears as '******' which is consistent with SHOW BACKUP and BACKUP. Epic: None

msbutler force-pushed the butler-show-inc branch from fef1dde to e6f47a0 Compare January 27, 2023 13:25

craig bot merged commit 5f0c56e into cockroachdb:master Jan 27, 2023

This was referenced Jan 28, 2023

PR #95562 - backupccl: sanitize ENCRYPTION_PASSPHRASE option in RESTORE in parser cockroachdb/docs#16145

Closed

PR #95562 - backupccl: explicitly parse SHOW BACKUP options cockroachdb/docs#16146

Open

msbutler deleted the butler-show-inc branch February 22, 2023 22:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

backupccl: explicitly parse SHOW BACKUP options #95562

backupccl: explicitly parse SHOW BACKUP options #95562

msbutler commented Jan 19, 2023 •

edited

Loading

blathers-crl bot commented Jan 19, 2023

cockroach-teamcity commented Jan 19, 2023

benbardin commented Jan 23, 2023

msbutler commented Jan 23, 2023

benbardin left a comment

stevendanna left a comment

stevendanna Jan 26, 2023

msbutler Jan 26, 2023

msbutler Jan 26, 2023

stevendanna Jan 26, 2023

msbutler Jan 26, 2023

msbutler left a comment

msbutler Jan 26, 2023

msbutler Jan 26, 2023

msbutler commented Jan 26, 2023

msbutler commented Jan 26, 2023

stevendanna commented Jan 27, 2023

msbutler commented Jan 27, 2023

craig bot commented Jan 27, 2023

craig bot commented Jan 27, 2023

backupccl: explicitly parse SHOW BACKUP options #95562

backupccl: explicitly parse SHOW BACKUP options #95562

Conversation

msbutler commented Jan 19, 2023 • edited Loading

blathers-crl bot commented Jan 19, 2023

cockroach-teamcity commented Jan 19, 2023

benbardin commented Jan 23, 2023

msbutler commented Jan 23, 2023

benbardin left a comment

Choose a reason for hiding this comment

stevendanna left a comment

Choose a reason for hiding this comment

stevendanna Jan 26, 2023

Choose a reason for hiding this comment

msbutler Jan 26, 2023

Choose a reason for hiding this comment

msbutler Jan 26, 2023

Choose a reason for hiding this comment

stevendanna Jan 26, 2023

Choose a reason for hiding this comment

msbutler Jan 26, 2023

Choose a reason for hiding this comment

msbutler left a comment

Choose a reason for hiding this comment

msbutler Jan 26, 2023

Choose a reason for hiding this comment

msbutler Jan 26, 2023

Choose a reason for hiding this comment

msbutler commented Jan 26, 2023

msbutler commented Jan 26, 2023

stevendanna commented Jan 27, 2023

msbutler commented Jan 27, 2023

craig bot commented Jan 27, 2023

craig bot commented Jan 27, 2023

msbutler commented Jan 19, 2023 •

edited

Loading