WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#10911 closed enhancement (fixed)

Optimize scan for lost attachments

Reported by: johnjosephbachir Owned by: 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 8 years ago.
Somewhat more simple and possibly more optimal SQL.
10911.diff (776 bytes) - added by Denis-de-Bernardy 8 years ago.
untested
10911.2.diff (871 bytes) - added by Denis-de-Bernardy 8 years ago.

Download all attachments as: .zip

Change History (22)

@johnjosephbachir
8 years ago

Somewhat more simple and possibly more optimal SQL.

#1 @scribu
8 years ago

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

It's certainly an improvement.

#2 @ryan
8 years ago

2.9 will require 4.1, so subquery away.

#3 @ryan
8 years ago

  • Milestone changed from 2.9 to 3.0

#4 @azaozz
8 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
8 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
8 years ago

  • Keywords needs-patch added; has-patch removed

@Denis-de-Bernardy
8 years ago

untested

#7 @Denis-de-Bernardy
8 years ago

  • Keywords has-patch added; needs-patch removed

#8 @Denis-de-Bernardy
8 years ago

  • Keywords tested added

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

#9 @westi
8 years ago

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

#11 @nacin
8 years ago

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

#12 @azaozz
8 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
8 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
8 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
8 years ago

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

#16 @nacin
8 years ago

  • Severity changed from blocker to major

Lowering severity after [14701].

#17 @nacin
8 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
8 years ago

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

#19 @nacin
8 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.