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: version of SHOW BACKUP that gives ownership data #57906

Closed
mwang1026 opened this issue Dec 14, 2020 · 2 comments · Fixed by #59732
Closed

backupccl: version of SHOW BACKUP that gives ownership data #57906

mwang1026 opened this issue Dec 14, 2020 · 2 comments · Fixed by #59732
Assignees
@Elliebababa
Labels
A-disaster-recovery C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-disaster-recovery

Comments

@mwang1026
Copy link

When users RESTORE we set the ownership of the restored object to the user running the RESTORE. That user then has the option to move ownership later on, but they don't have a way to see who the original owner was. We have SHOW BACKUP...WITH PRIVILEGES but that doesn't include owner information.

The goal here is to either extend WITH PRIVILEGES to include ownership, have a separate option for seeing ownership or having ownership as a column we show by default. For API stability reasons, we might want to hide it behind a flag but I'm open to suggesting.

** Alternative **

  • An option on RESTORE to restore with ownership retained. If there's a collision of ownership (e.g. user no longer exists) we should think through options. Options include ERROR, default to the user running restore, explicitly defined default, etc.
@mwang1026 mwang1026 added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-disaster-recovery T-disaster-recovery labels Dec 14, 2020
@kittydoor
Copy link

For my usecase, a way to restore users first, and then restore individual databases with ownership and grants preserved (with errors if something goes wrong) would be the best option. I can imagine there are different usecases too though.

@pbardea
Copy link
Contributor

pbardea commented Jan 28, 2021

With respect to the SHOW BACKUP changes, I think adding another column behind the WITH PRIVILEGES option makes the most sense to me.

Elliebababa added a commit to Elliebababa/cockroach that referenced this issue Feb 2, 2021
@Elliebababa
backupccl: add an owner column behind the WITH PRIVILEGES option
8639039 
Previously, when users perform RESTORE, they are ignorant of the original owner.

This PR gives ownership data as a column behind privileges.

Resolves: cockroachdb#57906.

Release note: None.
@Elliebababa Elliebababa mentioned this issue Feb 2, 2021
Merged
Elliebababa added a commit to Elliebababa/cockroach that referenced this issue Feb 2, 2021
@Elliebababa
backupccl: add an owner column behind the WITH PRIVILEGES option
251b442 
Previously, when users perform RESTORE, they are ignorant of the original owner.

This PR gives ownership data as a column behind privileges.

Resolves: cockroachdb#57906.

Release note(sql change): Update SHOW BACKUP ... WITH PRIVILEGES to display ownership information of objects in the backup.
craig bot pushed a commit that referenced this issue Feb 3, 2021
@tbg @Elliebababa
@RaduBerinde @otan
Merge #59647 #59732 #59746 #59747
cb9d41c 
59647: stop: rip out expensive task tracking r=knz a=tbg

First commit was put up for PR separately, ignore it here.

----

We are likely going to invest more in the stopper-conferred
observability in the near future as part of initiatives such as #58164,
but the task tracking that has been a part of the stopper since near
its conception has not proven to be useful in practice, while at the
same time raising concern about stopper use in hot paths.

When shutting down a running server, we don't particularly care about leaking
goroutines (as the process will end anyway). In tests, we want to ensure
goroutine hygiene, but if a test hangs during `Stop`, it is easier to look at
the stacks to find out why than to consult the task map.

Together, this left little reason to do anything more complicated than
what's left after this commit: we keep track of the running number of
tasks, and wait until this drops to zero.

With this change in, we should feel comfortable using the stopper
extensively and, for example, ensuring that any CRDB goroutine is
anchored in a Stopper task; this is the right approach for test flakes
such as in #51544 and makes sense for all of the reasons mentioned in
issue #58164 as well.

In a future change, we should make the Stopper more configurable and,
through this configurability, we could in principle bring a version of
the task map back (in debug builds) without backing it into the stopper,
though I don't anticipate that we'll want to.

Closes #52894.

Release note: None


59732: backupccl: add an owner column behind the WITH PRIVILEGES option r=pbardea a=Elliebababa

Previously, when users perform RESTORE, they are ignorant of the original owner.

This PR gives ownership data as a column behind privileges.

Resolves: #57906.

Release note: None.

59746: opt: switch checks to use CrdbTestBuild instead of RaceEnabled r=RaduBerinde a=RaduBerinde

The RaceEnabled flag is not very useful for checks; e.g. apparently
execbuilder tests aren't run routinely in race mode. These checks are
now "live" in any test build, using the crdb_test build tag.

Release note: None

59747: tree: correct StatementTag of ALTER TABLE ... LOCALITY r=ajstorm a=otan

Release note: None

Co-authored-by: Tobias Grieger <[email protected]>
Co-authored-by: elliebababa <[email protected]>
Co-authored-by: Radu Berinde <[email protected]>
Co-authored-by: Oliver Tan <[email protected]>
@craig craig bot closed this as completed in 8aa486f Feb 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Elliebababa Elliebababa

Labels
A-disaster-recovery C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-disaster-recovery
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

backupccl: add an owner column behind the WITH PRIVILEGES option Elliebababa/cockroach
4 participants
@pbardea @kittydoor @Elliebababa @mwang1026