#20838 closed enhancement (fixed)
Don't populate $col_info in production
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| 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)
Change History (14)
GrahamKelly
— 13 months ago
comment:1
johnbillion
— 13 months ago
comment:2
nacin
— 12 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
markjaquith
— 12 months ago
GrahamKelly — can you please share your profiling data?
comment:4
pento
— 11 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().
comment:5
pento
— 11 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
nacin
— 11 months ago
- Owner set to nacin
- Resolution set to fixed
- Status changed from new to closed
In [21472]:
comment:7
follow-up:
↓ 8
josephscott
— 11 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:
↓ 9
nacin
— 11 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
SergeyBiryukov
— 10 months ago
comment:10
nacin
— 10 months ago
In [21511]:
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.