Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
I'm not sure which way it is right, but the file INSTALL.mysql.txt recommends granting fewer privileges to the database than the online documentation. It appear other people than me had the same problem, see this recent bug-report.
Comment | File | Size | Author |
---|---|---|---|
#34 | core-1969108-mysql_txt-34.patch | 1.32 KB | cs_shadow |
#30 | core-1969108-mysql_txt-30.patch | 1.32 KB | cs_shadow |
#22 | INSTALL.mysql_.txt.patch | 1.34 KB | danblack |
Comments
Comment #1
neRok CreditAttribution: neRok commentedMoving to documentation.
Comment #2
jhodgdonI think that INSTALL.mysql.txt is wrong -- at least some contrib modules (not sure about core) require CREATE TEMPORARY TABLES permission for the MySQL user, and some contrib modules require LOCK TABLES. We should at least mention these as a good idea in INSTALL.mysql.txt (that bug report is in Commerce Kickstart by the way).
If we are going to change INSTALL.mysql.txt, we will need to do it in 8.x first (it has a pretty much if not entirely identical file).
Because I'm not entirely sure about this though, I'm temporarily setting the component of this issue to the database system so that those people can offer their opinion.
Comment #3
rooby CreditAttribution: rooby commentedIt would seem that CREATE TEMPORARY TABLES is required.
See db_query_temporary(), which has been around a long time.
However if you look at the drupal changelog there is this at
Drupal 6.0, 2008-02-13
"Drupal core no longer requires CREATE TEMPORARY TABLES or LOCK TABLES database rights."
I'm not sure if maybe #189837: Remove temporary table from comment.install had something to do with this?
But then later in the drupal 6 lifecycle there was #845542: db_query_temporary() will fail without the "CREATE TEMPORARY TABLES" with MySQL, documentation update is needed
And then I'm not sure at what stage we lost it again.
LOCK TABLES should at least be a recommendation if not a requirement.
Comment #4
danblack CreditAttribution: danblack commentedif contrib modules are using LOCK TABLES for anything other than backups chances are they should be using transactions. I wouldn't recommend granting LOCK TABLES.
Temporary tables do seem useful for some report generation modules and don't have as many side effects as LOCK TABLES so I can't see why this grant shouldn't be enabled.
Comment #5
jhodgdonWell, our database API definitely has db_query_temporary() and queryTemporary() methods. Core does not apparently use it, except for a few tests. And INSTALL.mysql.txt still does not mention the CREATE TEMPORARY TABLES permission.... I think that if our database API has functions that require this permission, we should mention it.
On the other hand, as far as I can tell, we do not have any uses of LOCK in Core, unless someone put it into a query without the ALL CAPS. I agree that we should not mention LOCK -- if a contrib module is using it for some reason, they should mention it in their own README.txt or INSTALL.txt file.
Comment #6
danblack CreditAttribution: danblack commentedCorrect, I couldn't find lock tables either. Added description about localhost in grant in attached patch
Comment #7
jhodgdonThanks for the patch!
Next time you submit a patch, please set the issue status to "Needs review", which alerts potential patch reviewers that there is a patch to review. :)
Can you move the comma you added to the previous line?
And maybe the line farther down in the file that says "Unless ... you will not be able to run Drupal" should also be revised? Because it is not strictly necessary for Drupal Core, as far as I can tell, to have CREATE TEMPORARY TABLES privs. So maybe this line should say something else, or have a special note about the temporary tables permission?
Comment #8
jhodgdonAlso, just as a note: we do not need to do this for the PostgreSQL install instructions, because in that file it tells you to create a user and a database, and make the user be the "owner" of the database, which automatically gives you all the privs, I believe. And SQLite is even simpler, because Drupal does all the setup, so no changes needed there either.
Comment #9
danblack CreditAttribution: danblack commentedack. Updated.
Comment #10
danblack CreditAttribution: danblack commentedComment #11
jhodgdonThanks! Looks pretty good. A couple of things to fix:
That is not actually accurate. It is the host for the MySQL database, which might not be the same as the Drupal site's hostname. (Sorry, missed that in the last review.)
And in this new change:
- What does "for every Drupal host" mean? I don't understand that phrase at all. Probably leave it out?
- You need a comma before "which".
- "currently optional" doesn't really tell me much. It is not used by Drupal core, but might be by contributed modules, so maybe it would be better to say "... which is not needed by Drupal core, but is needed for some contributed modules" and be explicit about it?
Comment #12
rooby CreditAttribution: rooby commentedIn addition to #11, why does it say:
"(except for CREATE TEMPORARY TABLES which is currently optional)"
When this issue is for saying that CREATE TEMPORARY TABLES is required.
There are drupal core functions that use CREATE TEMPORARY TABLES so it shouldn't be optional.
Comment #13
jhodgdonRE #12, as noted in previous comments, those Drupal Core functions that use temporary tables -- yes, they exist, but they are never actually called by Drupal Core. So if you are just running Drupal Core, you technically do not need temporary tables permission.
Comment #15
danblack CreditAttribution: danblack commentedNo, I'm actually correct on this one - http://dev.mysql.com/doc/refman/5.6/en/grant.html#grant-accounts-passwords
alternate - "every Drupal instance"? I was trying to highlight that if multiple web servers are installed you need to do the grant for each web server hostname/IP.
Ok with other comments.
Comment #16
rooby CreditAttribution: rooby commentedWhat if you run the tests?
Comment #17
danblack CreditAttribution: danblack commentedComment #18
danblack CreditAttribution: danblack commentedComment #21
jhodgdonSorry, you are right on #15 about localhost.
So...
I still am not extremely happy with this text. In #15 you say "I was trying to highlight that if multiple web servers are installed you need to do the grant for each web server hostname/IP."... OK, but ... the sentence is still confusing.
And I still think we need to mention contrib modules might need temporary tables.
So how about this, or something similar:
Note: Unless the database user/host combination for your Drupal installation has all of the privileges listed above (except possibly CREATE TEMPORARY TABLES, which is currently only used by Drupal core automated tests and some contributed modules), you will not be able to install or run Drupal.
Comment #22
danblack CreditAttribution: danblack commentedComment #23
jhodgdonThis looks good to me (not surprising!).
Provisionally setting to RTBC; if any of the other people involved in this issue have other comments, please change the status. If not, we can get this committed in a day or two. Thanks!
Comment #24
danblack CreditAttribution: danblack commentedhttps://drupal.org/documentation/install/create-database synced to same text.
Comment #25
danblack CreditAttribution: danblack commentedComment #26
jhodgdonThanks @danblack - looks good!
Comment #27
danblack CreditAttribution: danblack commentedmust be commit time now :-)
Comment #28
webchickLooks good to me, and seems jhodgdon signed off.
Committed and pushed to 8.x. Thanks!
Moving down to 7.x for the backport, which should be pretty easy, so tagging Novice.
Comment #30
cs_shadow CreditAttribution: cs_shadow commentedAttaching patch in #22 backported for 7.x
Comment #31
cs_shadow CreditAttribution: cs_shadow commentedComment #32
danblack CreditAttribution: danblack commentedwow, reviewed. Looks remarkably familiar.
Comment #33
jhodgdonUm, not quite! The 7.x patch is removing this line:
I do not think we want it to go away?
Comment #34
cs_shadow CreditAttribution: cs_shadow commentedRetained the 'databasename' line.
Comment #35
jhodgdonThanks! That looks better.
Comment #36
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks!