WordPress.org

Make WordPress Core

Opened 13 months ago

Closed 11 months ago

Last modified 10 months ago

#20838 closed enhancement (fixed)

Don't populate $col_info in production

Reported by: GrahamKelly Owned by: nacin
Priority: normal Milestone: 3.5
Component: Database Version: 3.3.2
Severity: normal Keywords: has-patch
Cc: joseph@…

Description

While profiling WordPress 3.3.2 I noticed that mysql_fetch_field() seemed to be fairly expensive. Upon looking into this I found that the col_info property is being set in wpdb but it does not appear (from some grepping through WP Core and quite a few extensions) that anyone is actually making use of the results (Either from directly accessing $col_info or from calls to get_col_info). In order to improve performance I wrapped the calls in 'if (WP_DEBUG)' which seems to do the trick.

Patch attached.

Attachments (3)

wpdb.patch (697 bytes) - added by GrahamKelly 13 months ago.
lazy-col-info.diff (2.1 KB) - added by pento 11 months ago.
lazy-col-info.2.diff (2.2 KB) - added by pento 11 months ago.

Download all attachments as: .zip

Change History (14)

GrahamKelly13 months ago

comment:1 johnbillion13 months ago

Out of curiosity, is your profiling data available? While it probably is a good idea to only fetch this while in debug mode (or maybe while SAVEQUERIES is true) I have previously profiled mysql_fetch_field and found the performance impact is negligable.

comment:2 nacin12 months ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from Awaiting Review to 3.5

This could be an easy lazy-loading win.

We could use a magic getter to handle someone accessing col_info directly, then make get_col_info() act on $this->result directly. This would prevent mysql_fetch_field() from firing unless someone needs it.

comment:3 markjaquith12 months ago

GrahamKelly — can you please share your profiling data?

pento11 months ago

comment:4 pento11 months ago

  • Keywords has-patch added; needs-patch removed

New patch uses a magic getter to only load column info when requested.

The only downside of this is that we can't do the mysql_free_result( $this->result ) in wpdb::query(), so I've moved it to wpdb::flush().

pento11 months ago

comment:5 pento11 months ago

Patch 2 changes the access visibility of wpdb::col_info and wpdb::get_col_info() to protected - they may need to be over-ridden by replacements for wpdb.

comment:6 nacin11 months ago

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

In [21472]:

Lazy-load column info in wpdb. props pento. fixes #20838.

comment:7 follow-up: josephscott11 months ago

  • Cc joseph@… added

The committed code - r21472 - includes error suppression:

@mysql_free_result( $this->result );

The PHP docs for mysql_free_result indicate that this is a problem only when a non-resource is provided:

If a non-resource is used for the result, an error of level E_WARNING will be emitted. It's worth noting that mysql_query() only returns a resource for SELECT, SHOW, EXPLAIN, and DESCRIBE queries.

Can we get a way with not having the @ with something like:

if ( is_resource( $this->result ) ) {
    mysql_free_result( $this->result );
}

?

It would be worth while to avoid the use of @ when ever possible.

comment:8 in reply to: ↑ 7 ; follow-up: nacin11 months ago

Replying to josephscott:

It would be worth while to avoid the use of @ when ever possible.

We discussed this as well. I agree this is a worthwhile change. New ticket?

comment:9 in reply to: ↑ 8 SergeyBiryukov10 months ago

Replying to nacin:

We discussed this as well. I agree this is a worthwhile change. New ticket?

#21533

comment:10 nacin10 months ago

In [21511]:

Eliminate error suppression for mysql_free_result() and only call it when the result is actually a resource. Depending on the query, mysql_query() can return a boolean rather than a resource, hence the original use of error suppression.

Fixes a warning introduced in [21472] when calling mysql_free_result() was moved to flush().

fixes #20838.

Note: See TracTickets for help on using tickets.