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

sql: add FLUSH PRIVILEGES hint #835

Merged
merged 6 commits into from
Jan 10, 2019
Merged

sql: add FLUSH PRIVILEGES hint #835

merged 6 commits into from
Jan 10, 2019

Conversation

lastzero
Copy link
Contributor

@lastzero lastzero commented Jan 3, 2019

It looked like FLUSH PRIVILEGES is not implemented because of pingcap/tidb#674. user-account-management.md also didn't mention it. I've added to all relevant pages.

@CLAassistant
Copy link

CLAassistant commented Jan 3, 2019 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@morgo
Copy link
Contributor

morgo commented Jan 3, 2019

May I request you make it clearer:

  • FLUSH PRIVILEGES is only required in the case of updates directly to the tables.
  • If you use GRANT, SET PASSWORD, DROP USER, CREATE USER etc, FLUSH PRIVILEGES is run automatically.

@lastzero
Copy link
Contributor Author

lastzero commented Jan 3, 2019

That's exactly what didn't work for me. Maybe it runs automatically, but not immediately?

Context: https://github.com/photoprism/photoprism/blob/develop/internal/tidb/init.go

@lastzero
Copy link
Contributor Author

lastzero commented Jan 3, 2019

If you are absolutely sure and it counts as a bug if it doesn't work immediately, I'll invest more time to debug the issue with unit tests 👍

@lonng
Copy link
Contributor

lonng commented Jan 3, 2019 •
edited
Loading

@morgo pingcap/tidb#8886

May I request you make it clearer:

  • FLUSH PRIVILEGES is only required in the case of updates directly to the tables.
  • If you use GRANT, SET PASSWORD, DROP USER, CREATE USER etc, FLUSH PRIVILEGES is run automatically.

Should execute FLUSH PRIVILEGES manually in the current release. If I am wrong, please point them out.

@lastzero
Copy link
Contributor Author

lastzero commented Jan 3, 2019

Should execute FLUSH PRIVILEGES manually in the current release. If I am wrong, please point them out.

This is also what your CTO told me. IMHO the docs should mention that, independent of what future releases might do. Does running FLUSH PRIVILEGE cause any harm I am not aware of?

@morgo
Copy link
Contributor

morgo commented Jan 3, 2019 •
edited
Loading

If you are absolutely sure and it counts as a bug if it doesn't work immediately, I'll invest more time to debug the issue with unit tests 👍

Yes, I worked on this code recently: pingcap/tidb#8886

When you run FLUSH PRIVILEGES, it sends a notification to etcd to update privileges. The bug I fixed was so that if there is no etcd (i.e. the test suite) it also runs FLUSH PRIVILEGES synchronously.

This is MySQL compatible behavior.

Does running FLUSH PRIVILEGE cause any harm I am not aware of?

Yes it can be an expensive command if there is a large number of users.

@lastzero
Copy link
Contributor Author

lastzero commented Jan 3, 2019

Ah, fixed 5 days ago? No wonder it doesn't work for me.

What about users that run older versions?

@morgo
Copy link
Contributor

morgo commented Jan 3, 2019

Ah, fixed 5 days ago? No wonder it doesn't work for me.

I fixed it for the case of the test suite 5 days ago, if you have etcd, the notification channel should still work. If it doesn't, that is a new bug.

What about users that run older versions?

The docs are internally versioned (although currently the website only shows 'master' - that is something that is being worked on). /cc @queenypingcap

@lastzero
Copy link
Contributor Author

lastzero commented Jan 3, 2019

We use the local version for our software. I know, special use case. We are a test suite for new ideas.

@lastzero
Copy link
Contributor Author

lastzero commented Jan 3, 2019

Also it seems there are ready-to-use Docker images out there, that use the local version. These people will waste a lot of time figuring out what's wrong, like I did. At least half a day.

@morgo
Copy link
Contributor

morgo commented Jan 3, 2019

We use the local version for our software. I know, special use case. We are a test suite for new ideas.

Do you mean tidb w/mocktikv (no tikv or pd)? If so, that was my motivation for fixing that bug :-) As well as this one pingcap/tidb#8836. I would love to chat more if you don't mind me emailing you.

Also it seems there are ready-to-use Docker images out there, that use the local version. These people will waste a lot of time figuring out what's wrong, like I did. At least half a day.

If it is documented as it is though, that means it is intended behavior. It is not intended, or MySQL compatible. We could change the wording and apply to non-master docs, or we could backport the fix to existing GA releases. I am fine with either approach.

@morgo
Copy link
Contributor

morgo commented Jan 4, 2019

We could change the wording and apply to non-master docs, or we could backport the fix to existing GA releases. I am fine with either approach.

I had a chat with @shenli. I will prepare a patch to backport to the 2.1 branch.

@morgo
Copy link
Contributor

morgo commented Jan 4, 2019

A backport to 2.1 is now pending review pingcap/tidb#8948

@lastzero
Copy link
Contributor Author

lastzero commented Jan 4, 2019

I would love to chat more if you don't mind me emailing you.

Sounds good! Let's have a chat, here are my contact details: https://blog.liquidbytes.net/contact/

@morgo
Copy link
Contributor

morgo commented Jan 4, 2019

Changes LGTM. Thanks for editing!

@morgo morgo requested a review from CaitinChen January 4, 2019 16:08
@CaitinChen CaitinChen changed the title Add FLUSH PRIVILEGES hint to user-account-management.md and FAQ.md sql: sdd FLUSH PRIVILEGES hint to user-account-management.md and FAQ.md Jan 8, 2019
@CaitinChen CaitinChen changed the title sql: sdd FLUSH PRIVILEGES hint to user-account-management.md and FAQ.md sql: add FLUSH PRIVILEGES hint to user-account-management.md and FAQ.md Jan 8, 2019
@CaitinChen CaitinChen changed the title sql: add FLUSH PRIVILEGES hint to user-account-management.md and FAQ.md sql: add FLUSH PRIVILEGES hint Jan 8, 2019
CaitinChen
CaitinChen reviewed Jan 9, 2019
View reviewed changes
sql/user-account-management.md Outdated

## Flush privileges

If you modified the grant tables directly, please run the following command to apply changes immediately:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
If you modified the grant tables directly, please run the following command to apply changes immediately:
If you modified the grant tables directly, run the following command to apply changes immediately:

Copy link
Contributor

Choose a reason for hiding this comment

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

We usually do not use "please" in technical documents.

sql/user-account-management.md Outdated
FLUSH PRIVILEGES;
```

See [Privilege Management](privilege.md) for details.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
See [Privilege Management](privilege.md) for details.
For details, see [Privilege Management](../sql/privilege.md).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is ../sql/ a mistake? Both documents are in sql, so ../sql is the same directory.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lastzero No, ../sql/ is not a mistake. Your link path is correct on GitHub, but on the PingCAP website it will be a broken link.
Our link rule: Any relative path that links to another file must start from the level of the root directory.
We updated the link path rule for SEO. Thanks for your cooperation :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting. You use pandoc, right? In my own projects, MkDocs automatically creates the right links in HTML as needed.

Copy link
Contributor

@CaitinChen CaitinChen Jan 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@lastzero We do use pandoc to convert HTML to PDF, but it has nothing to do with our links in the documents.
We have our own link rule because of the complex hierarchical relationship in our website.

@CaitinChen
Copy link
Contributor

@lastzero Thanks for your contribution!

@CaitinChen @lastzero
Use backticks to format title as code
2111b84 
Co-Authored-By: lastzero <michael@lastzero.net>
@CaitinChen CaitinChen mentioned this pull request Jan 9, 2019
Merged
CaitinChen
CaitinChen approved these changes Jan 10, 2019
View reviewed changes
Copy link
Contributor

@CaitinChen CaitinChen left a comment

Choose a reason for hiding this comment

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

LGTM

@CaitinChen CaitinChen merged commit 8ee8cdb into pingcap:master Jan 10, 2019
@sre-bot sre-bot added the contribution This PR is from a community contributor. label Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CaitinChen CaitinChen CaitinChen approved these changes

Assignees
No one assigned
Labels
contribution This PR is from a community contributor.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lastzero @CLAassistant @morgo @lonng @CaitinChen @sre-bot