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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

neRok’s picture

Version: 7.22 » 7.x-dev
Component: mysql database » documentation

Moving to documentation.

jhodgdon’s picture

Version: 7.x-dev » 8.x-dev
Component: documentation » database system
Issue tags: +Needs backport to D7

I 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.

rooby’s picture

It 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.

danblack’s picture

Issue summary: View changes

if 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.

jhodgdon’s picture

Title: INSTALL.mysql.txt does not mention LOCK TABLES, CREATE TEMPORARY TABLES » INSTALL.mysql.txt does not mention CREATE TEMPORARY TABLES and it should

Well, 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.

danblack’s picture

FileSize
837 bytes

Correct, I couldn't find lock tables either. Added description about localhost in grant in attached patch

jhodgdon’s picture

Status: Active » Needs work

Thanks 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. :)

  GRANT SELECT, INSERT, UPDATE, DELETE, CREATE, DROP, INDEX, ALTER
-  ON databasename.*
+  , CREATE TEMPORARY TABLES ON databasename.*

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?

jhodgdon’s picture

Also, 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.

danblack’s picture

FileSize
1.24 KB

ack. Updated.

danblack’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

Thanks! Looks pretty good. A couple of things to fix:

+ 'localhost' is the host where Drupal is installed

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:

-Note: Unless your database user has all of the privileges listed above, you will
-not be able to run Drupal.
+Note: Unless your database user, for every Drupal host, has all of the
+privileges listed above (except for CREATE TEMPORARY TABLES which is currently
+optional), you will not be able to run Drupal.

- 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?

rooby’s picture

In 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.

jhodgdon’s picture

RE #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.

The last submitted patch, 9: INSTALL.mysql_.txt.patch, failed testing.

danblack’s picture

+ 'localhost' is the host where Drupal is installed

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.)

No, I'm actually correct on this one - http://dev.mysql.com/doc/refman/5.6/en/grant.html#grant-accounts-passwords

for every Drupal host

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.

rooby’s picture

RE #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.

What if you run the tests?

danblack’s picture

Status: Needs work » Needs review
FileSize
1.3 KB
danblack’s picture

FileSize
1.28 KB

The last submitted patch, 17: INSTALL.mysql_.txt.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 18: INSTALL.mysql_.txt.patch, failed testing.

jhodgdon’s picture

Sorry, you are right on #15 about localhost.

So...

+Note: Unless your database user, created for every web server host that has
+Drupal installed, has all of the privileges listed above (except for CREATE
+TEMPORARY TABLES, which is currently used by the Drupal test suite), you will
 not be able to run Drupal.

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.

danblack’s picture

Status: Needs work » Needs review
FileSize
1.34 KB
jhodgdon’s picture

Component: database system » documentation
Status: Needs review » Reviewed & tested by the community

This 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!

danblack’s picture

Component: documentation » database system
Status: Reviewed & tested by the community » Needs review
danblack’s picture

Status: Needs review » Reviewed & tested by the community
jhodgdon’s picture

Thanks @danblack - looks good!

danblack’s picture

must be commit time now :-)

webchick’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: +Novice

Looks 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.

  • Commit 286af3a on 8.x by webchick:
    Issue #1969108 by danblack, jhodgdon: INSTALL.mysql.txt does not mention...
cs_shadow’s picture

Attaching patch in #22 backported for 7.x

cs_shadow’s picture

Status: Patch (to be ported) » Needs review
danblack’s picture

Status: Needs review » Reviewed & tested by the community

wow, reviewed. Looks remarkably familiar.

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

Um, not quite! The 7.x patch is removing this line:

- 'databasename' is the name of your database

I do not think we want it to go away?

cs_shadow’s picture

Status: Needs work » Needs review
FileSize
1.32 KB

Retained the 'databasename' line.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! That looks better.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x - thanks!

  • Commit dd8e45a on 7.x by David_Rothstein:
    Issue #1969108 by danblack, cs_shadow | Robert S: INSTALL.mysql.txt does...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.