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

cli: allow dump to be used by any user with SELECT #7974

Merged
merged 1 commit into from
Jul 26, 2016
Merged

cli: allow dump to be used by any user with SELECT #7974

merged 1 commit into from
Jul 26, 2016

Conversation

maddyblue
Copy link
Contributor

@maddyblue maddyblue commented Jul 22, 2016

The previous version used a SELECT to fetch from the system.descriptor
table. This thus required SELECT on that table, which only root has. Instead
of adding some way around this like a new API call, use the existing SHOW
statements to fetch the same data (although not as elegantly).

Fixes #7964


This change is Reviewable

@tamird
Copy link
Contributor

tamird commented Jul 22, 2016

Why can't we give everyone SELECT on those tables?

On Jul 22, 2016 03:21, "Matt Jibson" notifications@github.com wrote:

The previous version used a SELECT to fetch from the system.descriptor
table. This thus required SELECT on that table, which only root has.
Instead
of adding some way around this like a new API call, use the existing SHOW
statements to fetch the same data (although not as elegantly).

Fixes #7964 #7964

You can view, comment on, or merge this pull request online at:

#7974
Commit Summary

  • cli: allow dump to be used by any user with SELECT

File Changes

Patch Links:


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#7974, or mute the thread
https://github.com/notifications/unsubscribe-auth/ABdsPJTnyqbZlcU5UmT6EqKaz0p5B1o_ks5qYG-AgaJpZM4JSgRM
.

@tbg
Copy link
Member

tbg commented Jul 22, 2016

Multi-tenancy wouldn't want to let every user see everyone else's schema, I think.

@tbg
Copy link
Member

tbg commented Jul 22, 2016

Whether that's it or not, a comment somewhere would be good.

@tamird
Copy link
Contributor

tamird commented Jul 22, 2016

Right, but then we'd also need to show non-root users to create tables.

On Jul 22, 2016 06:46, "Tobias Schottdorf" notifications@github.com wrote:

Whether that's it or not, a comment somewhere would be good.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#7974 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABdsPHS-PIuWYcdEQe4TEhwYVhs1YAQtks5qYJ9tgaJpZM4JSgRM
.

The previous version used a SELECT to fetch from the system.descriptor
table. This thus required SELECT on that table, which only root has. Instead
of adding some way around this like a new API call, use the existing SHOW
statements to fetch the same data (although not as elegantly).

Fixes #7964
@maddyblue
Copy link
Contributor Author

Added a comment about multi-tenancy. @tamir either you have a grammar error in your last comment or I don't understand it. Could you rephrase?


Review status: 0 of 1 files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from Reviewable

@tamird
Copy link
Contributor

tamird commented Jul 22, 2016

s/show/allow/

On Jul 22, 2016 15:04, "Matt Jibson" notifications@github.com wrote:

Added a comment about multi-tenancy. @tamir https://github.com/tamir
either you have a grammar error in your last comment or I don't understand

it. Could you rephrase?

Review status: 0 of 1 files reviewed at latest revision, all discussions

resolved, some commit checks pending.

Comments from Reviewable
https://reviewable.io:443/reviews/cockroachdb/cockroach/7974#-:-KNIzOcTA5SlEmRuanUW:-216303748


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#7974 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABdsPPJMrAd9EqHRFw4aCN9hnbKxTrrKks5qYRQkgaJpZM4JSgRM
.

@tbg
Copy link
Member

tbg commented Jul 23, 2016

LGTM, though I didn't dig into the details so someone more qualified should.


Reviewed 1 of 1 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@tbg
Copy link
Member

tbg commented Jul 23, 2016

BTW, add a TODO (or issue) that uses information_schema instead once the RFC #7965 is implemented.

@tbg tbg mentioned this pull request Jul 23, 2016
@knz
Copy link
Contributor

knz commented Jul 26, 2016

LGTM. +1 on a TODO referring to information_schema.


Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@jseldess
Copy link
Contributor

Docs updated with cockroachdb/docs#511

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

Successfully merging this pull request may close these issues.

5 participants