Problem/Motivation

The path 'aggregator' shows a paginated list of feed items. The query involved however is unindexed even though it would be able to use one. With low amount of items this might not be too much issue, but when there are lots, it could cause issues. Plus due to the pagination there are links on every page to visit the last page and when search bots / spiders hit all those links it could affect the site's performance.

Proposed resolution

Adding an index to the table aggregator_item on the timestamp column would allow an index to be used for the query in question.

CommentFileSizeAuthor
#14 add-index-to-aggregator-item-table-d8-1790298-14.patch820 bytespwolanin
#9 add-index-to-aggregator-item-table-d8-1790298-9.patch486 bytespwolanin
#7 add-index-to-aggregator-item-table-d7-1790298-7.patch798 bytesnagba
#5 add-index-to-aggregator-item-table-d7-1790298-5.patch804 bytesnagba
#3 add-index-to-aggregator-item-table-d7-1790298-3.patch836 bytesnagba
#2 add-index-to-aggregator-item-table-1790298-2.patch737 bytesnagba
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nagba’s picture

nagba CreditAttribution: nagba commented
Status: Active » Needs review

The patch adds an index on the field 'timestamp' to the 'aggregator_item' table.

nagba’s picture

nagba CreditAttribution: nagba commented
FileSize
add-index-to-aggregator-item-table-1790298-2.patch737 bytes

hope attaching works this time

nagba’s picture

nagba CreditAttribution: nagba commented
FileSize
add-index-to-aggregator-item-table-d7-1790298-3.patch836 bytes

making the patch bulletproof

Status: Needs review » Needs work

The last submitted patch, add-index-to-aggregator-item-table-d7-1790298-3.patch, failed testing.

nagba’s picture

nagba CreditAttribution: nagba commented
Status: Needs work » Needs review
FileSize
add-index-to-aggregator-item-table-d7-1790298-5.patch804 bytes

fixed the patch

nagba’s picture

nagba CreditAttribution: nagba commented
Title: Unindexed » Unindexed query in aggregator module

fixed up title

nagba’s picture

nagba CreditAttribution: nagba commented
FileSize
add-index-to-aggregator-item-table-d7-1790298-7.patch798 bytes

rerolling patch for D7.18

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Version: 7.x-dev » 8.x-dev
Issue summary: View changes
FileSize
add-index-to-aggregator-item-table-d8-1790298-9.patch486 bytes

Should get the fix into D8

nagba’s picture

nagba CreditAttribution: nagba commented

patch looks good. As Peter and me looked, D8 still uses the same kind of query in ItemStorageController.

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Status: Needs review » Reviewed & tested by the community

makes sense

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Status: Reviewed & tested by the community » Needs work

ah..seems that we need an update function that adds the index, according to this https://drupal.org/comment/8264991#comment-8264991
i thought we didnt, sorry

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

oh, huh. I'd also though it was unneeded.

Guess we'll have to wrap it using a db_index_exists() check?

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs work » Reviewed & tested by the community
FileSize
add-index-to-aggregator-item-table-d8-1790298-14.patch820 bytes

Should be a straight port of the D7 patch then. back to RTBC (assuming tests pass).

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed abdb0c5 and pushed to 8.x. Thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Version: 8.x-dev » 7.x-dev
Status: Fixed » Patch (to be ported)
dcam’s picture

dcam CreditAttribution: dcam commented

#7 still applies locally. I'm going to retest it.

dcam’s picture

dcam CreditAttribution: dcam commented

7: add-index-to-aggregator-item-table-d7-1790298-7.patch queued for re-testing.

dcam’s picture

dcam CreditAttribution: dcam commented
Status: Patch (to be ported) » Reviewed & tested by the community

#7 is RTBC for D7. I contains the same changes that went into D8. I tested the patch to make sure the new index is added on a clean install of D7 and when updating an existing install. The index was correctly added in both cases.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: add-index-to-aggregator-item-table-d8-1790298-14.patch, failed testing.

dcam’s picture

dcam CreditAttribution: dcam commented
Status: Needs work » Reviewed & tested by the community

Again, it's #7 that is RTBC for D7.

Were there changes to Testbot's behavior so that it tests the last patch when the issue status is set to RTBC? I don't know why it tried to retest #14.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented
Status: Reviewed & tested by the community » Fixed

Committed to 7.x - thanks!

  • Commit 00e500b on 7.x by David_Rothstein: 
    Issue #1790298 by nagba, pwolanin: Unindexed query in aggregator module.

Status: Fixed » Closed (fixed)

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