Make WordPress Core

Opened 14 years ago

Closed 14 years ago

Last modified 14 years ago

#10911 closed enhancement (fixed)

Optimize scan for lost attachments

Reported by: johnjosephbachir's profile johnjosephbachir Owned by: westi's profile westi
Milestone: 3.0 Priority: normal
Severity: major Version: 2.8.4
Component: Optimization Keywords: has-patch tested
Focuses: Cc:

Description

This patch moves all of the logic for scanning for lost attachments to the DB.

The code is more simple, and from my simple but repeated tests, the performance is significantly improved (done against a pretty sizeable posts table).

We could do it in one query instead of two if WordPress required MySQL 4.1, but subqeuries are not supported in 4.0 so we have to do it in two.

The patch was made against trunk, revision 11997 (last change to update.php, 11013)

Let me know if you have any questions or suggestions.

John

Attachments (3)

upload.php.patch (1.0 KB) - added by johnjosephbachir 14 years ago.
Somewhat more simple and possibly more optimal SQL.
10911.diff (776 bytes) - added by Denis-de-Bernardy 14 years ago.
untested
10911.2.diff (871 bytes) - added by Denis-de-Bernardy 14 years ago.

Download all attachments as: .zip

Change History (22)

@johnjosephbachir
14 years ago

Somewhat more simple and possibly more optimal SQL.

#1 @scribu
14 years ago

  • Keywords has-patch added
  • Milestone changed from 2.8.5 to 2.9

It's certainly an improvement.

#2 @ryan
14 years ago

2.9 will require 4.1, so subquery away.

#3 @ryan
14 years ago

  • Milestone changed from 2.9 to 3.0

#4 @azaozz
14 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [12655]) Optimize scan for lost attachments, props johnjosephbachir, fixes #10911

#5 @Denis-de-Bernardy
14 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

we should use a subquery here, and the code is not custom post type safe.

#6 @scribu
14 years ago

  • Keywords needs-patch added; has-patch removed

@Denis-de-Bernardy
14 years ago

untested

#7 @Denis-de-Bernardy
14 years ago

  • Keywords has-patch added; needs-patch removed

#8 @Denis-de-Bernardy
14 years ago

  • Keywords tested added

new patch strips parent-less attachments like the original query. see also #12448, on the same screen.

#9 @westi
14 years ago

Can't test this it appears the scan is completely broken at the moment

#11 @nacin
14 years ago

  • Owner set to westi
  • Status changed from reopened to assigned

#12 @azaozz
14 years ago

Initially this scan was designed to catch attachments where the parent post is deleted, i.e. post_parent > 0 but post doesn't exist, not orphan attachments with post_parent = 0.

#13 @markjaquith
14 years ago

Denis — open a new ticket if you want to advocate for including post_parent = 0.

Is the optimization portion of this done? Or did we want to combine into one query, using a subquery?

#14 @nacin
14 years ago

  • Severity changed from normal to blocker

The lack of custom post type awareness here causes any attachments to a custom post type be labeled as "lost".

The existing patch, assuming it works properly and within the bounds of defining what a "lost" attachment is, seems like the best solution.

#15 @westi
14 years ago

(In [14701]) First pass at making the scan for lost attachments custom post type aware. See #10911.

#16 @nacin
14 years ago

  • Severity changed from blocker to major

Lowering severity after [14701].

#17 @nacin
14 years ago

The UX for lost attachments in general needs a little work, but that can wait. When you click "Scan", it refreshes the page, keeping you on the Untattached filter and not saying that you are viewing/searching lost attachments. If there are any found, it'll list those rows, otherwise, it'll list the unattached media. I digress.

Nonetheless, there should be limited unattached media as long as plugins and core cleans up after themselves.

However, the commit does introduce a quirk -- I personally find it a feature in more use cases than not, and think the ticket is ready to be closed as fixed at this point -- that a post type that has since been unregistered (say, plugin deactivated) will obviously not be in the query, and thus all attachments on those posts will be considered "lost."

The alternative is to simply do a post_type <> 'attachment' deal. But I'm not sure the current behavior shouldn't also be the intended behavior.

#18 @nacin
14 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

#19 @nacin
14 years ago

(In [14789]) Exclusion instead of inclusion for post types for lost attachment scan. Exclude attachments and any !public types, and don't consider attachments on unregistered post types to be lost. fixes #10911.

Note: See TracTickets for help on using tickets.