taxonomy_update_7005() is a batched update, running

    $query = 'SELECT td.vid AS vocab_id, td.tid, tn.nid, tn.vid, n.type, n2.created, n2.sticky, n2.nid AS is_current FROM {taxonomy_term_data} td INNER JOIN {taxonomy_term_node} tn ON td.tid = tn.tid INNER JOIN {node} n ON tn.nid = n.nid LEFT JOIN {node} n2 ON tn.vid = n2.vid ORDER BY tn.vid, td.weight ASC, tn.tid';

for every batch. This query is slow and I think adds up to a lot of time. Drupal.org uses 1418 batches.

We should do this query once and store the data.

CommentFileSizeAuthor
#11 1549390.patch6.27 KBBTMash
#10 1549390.patch6.32 KBBTMash
#3 1549390.patch6.37 KBdrumm
#1 1549390.patch6.72 KBdrumm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drumm’s picture

Status: Active » Needs review
Issue tags: +drupal.org D7
FileSize
6.72 KB

Let's see if this untested patch works.

Status: Needs review » Needs work

The last submitted patch, 1549390.patch, failed testing.

drumm’s picture

Status: Needs work » Needs review
FileSize
6.37 KB

Fix whitespace.

xjm’s picture

Status: Needs review » Needs work
+++ b/modules/taxonomy/taxonomy.installundefined
@@ -592,36 +592,107 @@ function taxonomy_update_7005(&$sandbox) {
+    $result = db_query_range('SELECT vocab_id, tid, nid, vid, type, created, sticky, is_current FROM {taxonomy_update_7005} ORDER BY n', $sandbox['last'], $batch);

"Order by n"? Typo?

xjm’s picture

Status: Needs work » Needs review

Alright, drumm clarified this in IRC. n is here:

+++ b/modules/taxonomy/taxonomy.installundefined
@@ -592,36 +592,107 @@ function taxonomy_update_7005(&$sandbox) {
+        'n' => array(
+          'description' => 'Preserve order.',
+          'type' => 'serial',
+          'unsigned' => TRUE,
+          'not null' => TRUE,
+        ),

Status: Needs review » Needs work
Issue tags: -drupal.org D7

The last submitted patch, 1549390.patch, failed testing.

BTMash’s picture

Status: Needs work » Needs review

#3: 1549390.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +drupal.org D7

The last submitted patch, 1549390.patch, failed testing.

drumm’s picture

I think the problem is that the taxonomy update exits unexpectedly somehow and leaves the rest of the updates undone.

BTMash’s picture

FileSize
6.32 KB

I went through this to see what is going on and what I found is that DBTNG is being strict about the query and there being null values for created, is_current, sticky. I moved the query out just so it was easier to debug which led me to the following error:

Failed: PDOException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'created' cannot be null: INSERT INTO {taxonomy_update_7005} (vocab_id, tid, nid, vid, type, created, sticky, is_current) SELECT td.vid AS vocab_id, td.tid AS tid, tn.nid AS nid, tn.vid AS vid, n.type AS type, n2.created AS created, n2.sticky AS sticky, n2.nid AS is_current FROM {taxonomy_term_data} td INNER JOIN {taxonomy_term_node} tn ON td.tid = tn.tid INNER JOIN {node} n ON tn.nid = n.nid LEFT OUTER JOIN {node} n2 ON tn.vid = n2.vid ORDER BY tn.vid ASC, td.weight ASC, tn.tid ASC; Array ( ) in taxonomy_update_7005() (line 689 of /Users/amodi/Sites/d7_dev/modules/taxonomy/taxonomy.install).

Attaching revised patch which still fails (thus not changing status).

BTMash’s picture

Status: Needs work » Needs review
FileSize
6.27 KB

This patch is with created, sticky, is_current being allowed to be null values. The test still fails thought efails are now on the taxonomy checks (so atleast it goes through the upgrade). Maybe testbot will give even more info?

drumm’s picture

Great, I can try this out next time we try a fresh upgrade of Drupal.org's DB. One small thing I noticed is the "This query must return a consistent ordering across multiple calls." comment can be removed or edited; we no longer run that slow query across multiple calls, and the order is still important.

BTMash’s picture

Wow...this is really strange. I had a massive number of errors popping up on my local installation so this *shouldn't* be working...I'll try and make some time to look into this over the afternoon.

BTMash’s picture

I took a look to see what was going on locally and its again due to #1504994: Upgrade tests for taxonomy are failing. So this patch is actually working correctly.

drumm’s picture

Issue tags: +porting

tag

drumm’s picture

Status: Needs review » Reviewed & tested by the community

I tested this on http://7.devdrupal.org/. Plenty of other stuff is broken, but posts still have taxonomy terms, like http://7.devdrupal.org/node/1548238. And the update finished approximately 20% faster, down to 7 hr 44 min. Details about the upgrade process as a whole are at #1559652: Get update.php running smoothly on 7.devdrupal.org

chx’s picture

Thanks for the patch. Aside from the uneasiness coming from changing an existing update function, this does seem to be OK and it's tested automatically and manually so I hope we do not open a can of worms again. Let's go with it.

drumm’s picture

All the logic should be the same, just rearranging so a slow query only needs to be done once. The riskiest part is the DBTNG conversion of $query.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

A little scary, but looks good to me too. And the fact that it's been well-tested is a good thing :)

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/a80c7e8

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