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

Jump to bottom

backupccl: invalid serialization of the AST #63216

Closed
knz opened this issue Apr 7, 2021 · 2 comments · Fixed by #69990
Closed

backupccl: invalid serialization of the AST #63216

knz opened this issue Apr 7, 2021 · 2 comments · Fixed by #69990
Assignees
@rhu713
Labels
A-disaster-recovery C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-disaster-recovery

Comments

@knz
Copy link
Contributor

knz commented Apr 7, 2021 •
edited by exalate-issue-sync bot
Loading

See this comment by @dt which points to this code:

   args.BackupStatement = tree.AsString(backupNode)

this is paired with the following code in schedule_exec.go:

  node, err := tree.ParseOne(args.BackupStatement)
  ...

The expectation is thus that ParseOne will restore the original statement. That is not true however; AsString implies the FmtSimple flag, which is meant for display for human users. It is not guaranteed to preserve the statement.

The model to follow is the one set by DistSQL; this needs to format using FmtParsable instead.

cc @pbardea @dt for triage

Epic CRDB-8816
@knz knz added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-disaster-recovery labels Apr 7, 2021
@dt
Copy link
Member

dt commented Apr 28, 2021

@knz you fixed this, right? it's now tree.AsStringWithFlags(backupNode, tree.FmtSimple|tree.FmtShowPasswords) ?

@knz
Copy link
Contributor Author

knz commented Apr 29, 2021

No it's still incorrect - we need FmtParsable|FmtShowPasswords

FmtSimple is lossy and generates unparsable stuff sometimes, even with passwords properly spelled out.

@dt dt removed their assignment Jun 1, 2021
@rhu713 rhu713 mentioned this issue Sep 9, 2021
Merged
craig bot pushed a commit that referenced this issue Sep 10, 2021
Merge #69990
9d5e6ab 
69990: backupccl: use FmtParsable when storing statement in scheduled backup r=rhu713 a=rhu713

Previously, the executed backup statement for schedules was serialized and
stored using FmtSimple. This format can be lossy and the preferred format to
retain all information is FmtParsable. This patch switches the serialization to
use FmtParsable.

Resolves #63216

Release justification: change should have no impact on current backup schedules.

Release note: None

Co-authored-by: Rui Hu <[email protected]>
@craig craig bot closed this as completed in c30d567 Sep 10, 2021
@rhu713 rhu713 self-assigned this Sep 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rhu713 rhu713

Labels
A-disaster-recovery C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-disaster-recovery
Projects
No open projects
Disaster Recovery Backlog
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

backupccl: use FmtParsable when storing statement in scheduled backup rhu713/cockroach
4 participants
@dt @knz @rhu713 @mwang1026