WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#35797 closed defect (bug) (fixed)

delete_metadata should only delete metadata cache entries with the value specified in $meta_value arg

Reported by: rahal.aboulfeth Owned by: boonebgorges
Milestone: 4.5 Priority: normal
Severity: normal Version: 3.3
Component: Options, Meta APIs Keywords: has-patch
Focuses: administration, performance Cc:

Description

In some cases ( huge database ) , wp_delete_attachment caused a Fatal error related to available memory.

The investigation led me to delete_metadata and more precisely to this line :

<?php
$object_ids = $wpdb->get_col( $wpdb->prepare( "SELECT $type_column FROM $table WHERE meta_key = %s", $meta_key ) );

Which as you see, tries to get all the post that have a meta_key.. ( in my case, it was the thumbnail ) , theses posts were extracted to allow flushing their meta cache.

Attachments (1)

meta.patch (629 bytes) - added by rahal.aboulfeth 6 years ago.
patch fix

Download all attachments as: .zip

Change History (8)

@rahal.aboulfeth
6 years ago

patch fix

#1 @rahal.aboulfeth
6 years ago

  • Focuses multisite removed

#2 @rahal.aboulfeth
6 years ago

  • Summary changed from delete_metadata should only delete metadata entries with the value specified in $meta_value arg to delete_metadata should only delete metadata cache entries with the value specified in $meta_value arg

#3 follow-up: @Clorith
6 years ago

  • Keywords has-patch added

I'm not sure your patch applies here, as it fires on the conditional that you've supplied delete_metadata with the $delete_all as true, and you are then meant to delete all entries with that key regardless of the meta_value.

I suspect it would probably make more sense to look into the logic of wp_delete_attachment here

#4 @rahal.aboulfeth
6 years ago

The $meta_value comment says : Metadata value. Must be serializable if non-scalar. If specified, only delete metadata entries with this value. Otherwise, delete all entries with the specified meta_key. Pass null, false`, or an empty string to skip this check. (For backward compatibility,it is not possible to pass an empty string to delete those entries with an empty string for a value.)

The $delete_all comment says : Optional, default is false. If true, delete matching metadata entries for all objects, ignoring the specified object_id. Otherwise, only delete matching metadata entries for the specified object_id.

If I understand correctly , the matching metadata applies to entries where both key and value match , and not only the key. The fact is, if we try using this call with the _thumbnail_id for example, will will certainly get consume all the memory with wpbd:get_col .

But I think we can use an other conditional to add "AND meta_value = %s" if the $meta_value is specified. what do you think?

#5 in reply to: ↑ 3 @boonebgorges
6 years ago

  • Milestone changed from Awaiting Review to 4.5
  • Owner set to boonebgorges
  • Status changed from new to assigned

@rahal.aboulfeth Thanks for the ticket and for the patch.

Replying to Clorith:

I'm not sure your patch applies here, as it fires on the conditional that you've supplied delete_metadata with the $delete_all as true, and you are then meant to delete all entries with that key regardless of the meta_value.

I suspect it would probably make more sense to look into the logic of wp_delete_attachment here

I believe this is incorrect. The "all" in $delete_all refers to "all objects" rather than "all meta for a given object".

#6 @boonebgorges
6 years ago

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

In 36511:

In delete_metadata(), only invalidate cache for affected objects.

The $delete_all flag in delete_metadata() triggers cache invalidation for
multiple objects. Previously, invalidation took place for all objects matching
the $meta_key parameter, regardless of whether $meta_value was also set.
This resulted in overly aggressive invalidation.

Props rahal.aboulfeth.
Fixes #35797.

#7 @johnbillion
6 years ago

  • Version changed from trunk to 3.3
Note: See TracTickets for help on using tickets.