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.The function http://api.drupal.org/api/function/search_touch_node/7 is used to let various operations say "This node has changed and needs to be reindexed".
It works by setting the .reindex field in {search_dataset} to the current request time. Then in node_update_index(), a query is run that orders nodes by their .reindex field, and picks out the lowest-numbered N nodes for reindexing.
The problem is that if a node has been previously marked for reindexing when search_touch_node() is called, search_touch_node() will set its reindex field to a higher number (because it's a later request), thereby lowering its priority for reindexing.
So instead, search_touch_node() should check to see if the reindex bit is already set to something non-zero, and not change it if it is. It should also only try to do the update if the node already exists in the table, probably? But that may be a moot point if #312395: Queries on search admin and node indexing are slow for many-node sites is fixed by making sure every node gets added to the table when first created anyway.
| Comment | File | Size | Author |
|---|---|---|---|
| #14 | 735154-with-tests-and-comment-14.patch | 1.98 KB | jhodgdon |
| #8 | 735154-test-only-FAIL-8.patch | 1.43 KB | jhodgdon |
Comments
Comment #1
jhodgdonThis applies to D8 too. The D8 function is more generic and is called search_mark_for_reindex(), but it does the same thing.
Comment #2
jhodgdonHere's a patch. I tested it manually and it works fine.
We have tests for reindexing, so hopefully they will still work. I guess we could add a new test for this behavior, but I'm not sure how necessary that is?
Comment #3
jhodgdon2: 735154.patch queued for re-testing.
Comment #4
jhodgdon2: 735154.patch queued for re-testing.
Comment #5
jhodgdon2: 735154.patch queued for re-testing.
Comment #6
pwolanin CreditAttribution: pwolanin commentedIt would seem a lot easier to add a condition to the update query?
I'm not clear why you need the GROUP BY if the reindex query always sets items with the same sid to the same timestamp?
Also, it would be helpful to have a test of the logic here?
Comment #7
jhodgdonDoh! I like the patch in #6. A test would indeed be a good idea. I'll get on that.
Comment #8
jhodgdonOK! Here's the (one-line, ultra-simple, really great) patch from pwolanin #6, with a test added, plus a test-only patch that fails.
Assuming that the test bot agrees with my local test results (test fails without patch, passes with), I would be +1 on RTBC. Someone other than me will need to give the test a quick review.
And if you're wondering why the test is in the "Mutlilingual" test, that is where we are currently testing things related to reindexing and throttling, for what it's worth. I just added a few more lines to the existing test there. Probably we should rename that test, but I think that's a separate issue.
Comment #10
jhodgdon8: 735154-with-tests-8.patch queued for re-testing.
Comment #12
jhodgdonOK, the tests passed/failed as expected. I endorse the code fix from pwolanin for RTBC. Anyone want to review the test I wrote so we can get this easy change in?
Comment #13
pwolanin CreditAttribution: pwolanin commentedWe should probably add a code comment explaining why the condition is there?
Comment #14
jhodgdonGood idea. Here's one more patch, this time with a 2-line comment. Small patch, didn't bother with interdiff, hope that is OK. The only difference is the new comment in search_mark_for_reindex().
Comment #15
Nick_vhI'm not entirely sure about the reindex column but it's looking good and it's a simple patch.
Comment #16
Nick_vhComment #17
jhodgdonComment #18
alexpottCommitted 9f92f4e and pushed to 8.x. Thanks!
Comment #20
jhodgdon