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.

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 support for SHOW BACKUP WITH SCHEMAS #39323

Merged
merged 1 commit into from
Aug 7, 2019

Conversation

pbardea
Copy link
Contributor

@pbardea pbardea commented Aug 5, 2019

Add the ability to show the create statement for the tables, views
or sequences that are stored within a backup. Tables with references to
foreign keys will only display foreign key constraints if the table to
which the constraint relates to is also in the backup.

Closes #38517.

The SQL syntax chose was SHOW BACKUP WITH SCHEMAS. The
changes for this were minimal since they were all already keywords.
Note that WITH SCHEMAS cannot be used along with the RANGES
and FILES descriptor.

Release note (enterprise change): Add support for displaying creation
statements of relations stored in a backup.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@pbardea pbardea changed the title [wip] backupccl: add support for SHOW BAKCUP WITH SCHEMAS backupccl: add support for SHOW BAKCUP WITH SCHEMAS Aug 5, 2019
@pbardea
Copy link
Contributor Author

pbardea commented Aug 5, 2019

bors try

@craig
Copy link
Contributor

craig bot commented Aug 5, 2019

🔒 Permission denied

Existing reviewers: click here to make pbardea a reviewer

@pbardea
Copy link
Contributor Author

pbardea commented Aug 5, 2019

bors try

craig bot pushed a commit that referenced this pull request Aug 5, 2019
@pbardea pbardea requested review from a team August 5, 2019 16:36
@craig
Copy link
Contributor

craig bot commented Aug 5, 2019

try

Build succeeded

@pbardea pbardea requested a review from dt August 5, 2019 17:12
@pbardea pbardea changed the title backupccl: add support for SHOW BAKCUP WITH SCHEMAS backupccl: add support for SHOW BACKUP WITH SCHEMAS Aug 5, 2019
@pbardea pbardea force-pushed the show-backup-with-schemas branch 2 times, most recently from 2190d17 to ad31f02 Compare August 5, 2019 18:13
Copy link
Member

@dt dt left a comment

Choose a reason for hiding this comment

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

LGTM on the backup parts and tests.

I'd defer to @knz the the change in resolver.go since that's his area.

@pbardea pbardea requested a review from knz August 5, 2019 19:46
Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

Reviewed 10 of 10 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @pbardea)


pkg/sql/parser/sql.y, line 3303 at r2 (raw file):

    }
  }
| SHOW BACKUP WITH SCHEMAS string_or_placeholder
  1. why not show backup schemas and why is with important

  2. the thing is not showing schemas, it's showing CREATE statements. Maybe show backup ddl is the better phrasing.

  3. I am sad that these are becoming separate things. What if I want to see both the files and the create statements? Maybe these things should be options that can be combined.

Copy link
Member

@dt dt left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 10 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rolandcrosby)


pkg/sql/parser/sql.y, line 3303 at r2 (raw file):

Previously, knz (kena) wrote…
  1. why not show backup schemas and why is with important

  2. the thing is not showing schemas, it's showing CREATE statements. Maybe show backup ddl is the better phrasing.

  3. I am sad that these are becoming separate things. What if I want to see both the files and the create statements? Maybe these things should be options that can be combined.

re: 3, I think I prefer this direction as-is: files and ranges are pretty much only debugging options and there is almost no reason for an end-user to run them, and particularly little reason they'd want to combine them with other options. For such an edge case, I'm fine with asking you to just run it again with the other option (in particularly since they return different rows -- i.e. one per table or one per range or one per file).

re: 2: I have a slight preference for SCHEMAS this over DDLS since the later I think is somewhat more obscure, but I'll let @rolandcrosby chime in.

@pbardea
Copy link
Contributor Author

pbardea commented Aug 5, 2019

  1. I thought it made sense since SHOW BACKUP WITH SCHEMAS displays the contents of SHOW BACKUP along with an additional row.

  2. I personally don't have a strong preference as long as the feature is documented. An alternative syntax could be SHOW BACKUP WITH CREATE.

But on the points regarding the naming I welcome @rolandcrosby's input as well.

  1. The main reason I didn't include it as an option for RANGES and FILES was the fact that they both group data by a key range rather than a relation, so the concepts didn't map well.

@thoszhang
Copy link
Contributor

Should this also omit views referencing tables that are not in the backup?

@dt
Copy link
Member

dt commented Aug 6, 2019

I think printing the views is actually one of the big motivating reasons for this change: it lets you recreate a view by hand that RESTORE won't touch due to missing one of the referenced tables.

@thoszhang
Copy link
Contributor

That makes sense. There's no way to do the same for FKs because the backups don't have the names of the referenced table/columns, right?

@pbardea
Copy link
Contributor Author

pbardea commented Aug 6, 2019

@lucy-zhang Yes, when getting the CREATE statement for the view it can use desc.ViewQuery, which stores the original query itself as a string. When getting the CREATE statement for a table, FKs are references to other descriptors which need to be looked up through a lookup context (lCtx). I think the idea is that the references may not be valid when restoring, so that information is essentially lost in the backup.

@rolandcrosby
Copy link

Yup, the view issue that @pbardea identified is indeed part of what spurred us to prioritize this work. A customer was unable to restore the entirety of a backup because it contained a view referencing a non-backed-up table, and we realized that there wasn't a good way to retrieve the view definition.

Re: @knz's syntax questions, I don't particularly care if we include WITH in the grammar or not so will defer to him. I have a slight preference for "schemas" over "ddls" for the same reason as David, and would point out that we use "schema" as an overarching term for various DDL-related stuff (e.g. the act of adding an index or constraint is considered a "schema change").

@knz
Copy link
Contributor

knz commented Aug 6, 2019

No further comments 👍

Add the ability to show the create statement for the tables, views
or sequences that are stored within a backup. Tables with references to
foreign keys will only display foreign key constraints if the table to
which the constraint relates to is also in the backup.

Closes cockroachdb#38517.

Release note (enterprise change): Add support for displaying creation
statements of relations stored in a backup.
@pbardea
Copy link
Contributor Author

pbardea commented Aug 7, 2019

Looks like the general consensus is that the WITH is superfluous and SCHEMAS is preferred over DDLS. So I updated the grammar to be:

SHOW BACKUP SCHEMAS [backup_path]

@pbardea
Copy link
Contributor Author

pbardea commented Aug 7, 2019

bors r+

craig bot pushed a commit that referenced this pull request Aug 7, 2019
39323: backupccl: add support for SHOW BACKUP WITH SCHEMAS r=pbardea a=pbardea

Add the ability to show the create statement for the tables, views
or sequences that are stored within a backup. Tables with references to
foreign keys will only display foreign key constraints if the table to
which the constraint relates to is also in the backup.

Closes #38517.

The SQL syntax chose was `SHOW BACKUP WITH SCHEMAS`. The
changes for this were minimal since they were all already keywords. 
Note that `WITH SCHEMAS` cannot be used along with the `RANGES`
and `FILES` descriptor.

Release note (enterprise change): Add support for displaying creation
statements of relations stored in a backup.

Co-authored-by: Paul Bardea <pbardea@gmail.com>
@craig
Copy link
Contributor

craig bot commented Aug 7, 2019

Build succeeded

@craig craig bot merged commit 06ad5eb into cockroachdb:master Aug 7, 2019
@pbardea pbardea deleted the show-backup-with-schemas branch August 7, 2019 19:12
@kenliu
Copy link

kenliu commented Aug 7, 2019

nice work @pbardea

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

backupccl: enable SHOW BACKUP to display schemas
7 participants