I noticed this slow query on my site which is not hitting indexes:
SELECT base_table.vid AS vid, base_table.nid AS nid FROM node base_table INNER JOIN node__feeds_item node__feeds_item ON node__feeds_item.entity_id = base_table.nid INNER JOIN node_field_data node_field_data ON node_field_data.nid = base_table.nid WHERE (node__feeds_item.feeds_item_guid = '32_db5dcdf1d298bfe678f24422e21c8e97') AND (node_field_data.type = 'job_per_template') GROUP BY base_table.vid, base_table.nid LIMIT 1
I tracked it down to FieldTargetBase::getUniqueValue() where a condition for the query is not taken info account: the feed ID is not taken into account at all.
megachriz says this is intentional by accident and cannot be changed for backwards compatibility. That means we need a DB index on the GUID alone for acceptable query speed when there are many items.
Create a lot imported node entities on your site. I saw the performance problem with 700k nodes.
Add DB index to the GUID columns alone, then the query is fast.
Merge request review
none
none
none
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | feeds-3566159-15.patch | 4 KB | klausi |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
megachrizI think that with the current implementation you can update content created by an other feed as well. So changing the query could break certain workflows.
Comment #3
klausiAha ok, that is not a use case for us. A node entity always belongs to one feed in our use case.
Then I think we have to override the method getUniqueValue() in our custom JobiqoFeedsItem class.
That would also explain why my colleagues developed custom code to prefix the feed item guid with the feed ID.
Comment #4
megachrizI've done that as well on a few sites. 😅
Comment #5
klausiMaybe the DB index design of the feeds_item table is not correct? If the guid of a feed item can be considered over all feeds for lookup then there needs to be a DB index on the guid field feeds_item_guid. Currently there is only a combined index of (guid + feed ID).
In this case the bundle is also added to the query, but I assume that does not matter if there is an index on guid.
Comment #6
klausiConfirmed, I added a DB index manually to my database on the feeds_item_guid column, then the query from above is fast.
Should I make a merge request to add that to Feeds? Is probably relevant for all sites that have a lot feeds items.
Comment #7
megachrizYes, that would be great to add an index for feeds_item_guid!
And perhaps the fact that the feed ID was not included in the query was not intentional. Because in the D7 version of Feeds, GUID was not globally unique, but unique per importer. The discussion in #1539224: Add support for unique fields to be unique site wide also suggests that it wasn't intentional. But I think there's no going back now, because of the risk of breaking people's workflows.
Comment #9
klausiDone. Not super happy about the update function, let me know if there is a better way to add a DB index.
Comment #10
klausiupdated issue summary
Comment #11
megachrizIt could also be done with a post update, but the implementation would probably be similar.
With manual testing I discovered that the index doesn't get added to revision tables when running database updates.
For new feeds_item fields, the index does get added to revision tables.
I've been trying to add tests for this update, but it is complicated. The test pass even when checking the node_revision__feeds_item table after applying the update. But with manual testing, the index is not added.
Comment #12
klausiGood point about the revision table, I also added the same index there now.
Not sure about the automated test and if it is required. The update function is not likely to change in the future. That the test is passing is a good sign in any case, maybe some combination of update and cache clear creates the index automatically.
So for me it would be fine to merge as is now.
Comment #13
klausiBetter title.
Comment #14
klausiUploading stable patch file for composer patches.
Comment #15
klausiUploading stable patch file without binary test file and test changes for composer patches.
Comment #16
megachrizI tested the new update manually on a site with feed types for many different entity types. With one exception, all feeds_item and revision__feeds_item were updated and got the new index. The only table not updated on the site that I tested the update function on, was called "old_b9a028taxonomy_term" and that appears to be a left-over table from a very old Drupal database update, as described in https://www.drupal.org/node/3046576, so it makes sense that this one wasn't updated.
I removed the test as it wasn't testing the update properly and also because I used a whole Drupal 10 database dump for it, it could possibly cause issues in future Drupal versions - when Drupal Core has removed necessary updates to upgrade from Drupal 10.
I scheduled the merge. Thanks @klausi for the improvement!
Comment #19
klausiThanks a lot for merging, you are the best!