WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

Last modified 17 months ago

#11622 closed enhancement (maybelater)

switch to PDO::prepare when we require PHP 5.1

Reported by: sirzooro Owned by: ryan
Milestone: Priority: normal
Severity: normal Version:
Component: Database Keywords:
Focuses: Cc:

Description

This discussion started in #11608:

(me): Replying to Denis-de-Bernardy:

In retrospect we should have used the syntax that is accepted by PDO for this stuff, i.e. either of: "SELECT * FROM foo WHERE bar = ?" # ? gets replaced by first variable "SELECT * FROM foo WHERE bar = :bar" # :bar gets replaced by variable named bar

We can consider adding new function prepare2() which will use PDO-like syntax, and mark prepare() as deprecated. This will require a lot of work (quick search showed 43 files), but we will avoid such problems in the future. This can be done under another ticket.

miqrogroove:

If you're going to re-invent the wheel, query parsing seems like a bad approach in general, and that particular syntax improvement offers very marginal benefit above the needed documentation.

hakre:

I think it's a good approach to shift away from the broken design.

Change History (26)

comment:1 miqrogroove4 years ago

  • Cc miqrogroove@… added

comment:2 nacin4 years ago

I'd like to see how this plays out, but just a thought, if this is done, pdo_prepare() or prepare_pdo() would make more sense than prepare2().

comment:3 Denis-de-Bernardy4 years ago

prepare_sql() maybe. :-)

comment:4 follow-up: filosofo4 years ago

Before you put too much work into this, let's push to get PHP minimum for WP 3.0 raised to PHP 5.1 (or better, 5.2) and then we can actually use PDO instead of trying to imitate it.

comment:5 Denis-de-Bernardy4 years ago

  • Milestone changed from 3.0 to Future Release
  • Summary changed from Implement wpdb::prepare2() which will use PDO-like syntax to switch to PDO::prepare when we require PHP 5.1

agreed...

comment:6 in reply to: ↑ 4 hakre4 years ago

Replying to filosofo:

Before you put too much work into this, let's push to get PHP minimum for WP 3.0 raised to PHP 5.1 (or better, 5.2) and then we can actually use PDO instead of trying to imitate it.

Maybe you should knock on Janes door with that, she opened the forums.

comment:7 hakre4 years ago

on wishlist30 for dancole

comment:8 hakre4 years ago

Ignore last comment please, comment in error.

comment:9 follow-up: josephscott4 years ago

By that time we can just use mysqli and the built in prepare function - http://us.php.net/manual/en/mysqli.prepare.php

comment:10 in reply to: ↑ 9 ; follow-ups: hakre4 years ago

Replying to josephscott:

By that time we can just use mysqli and the built in prepare function - http://us.php.net/manual/en/mysqli.prepare.php

The switch to PDO would offer the benefit that at least some kind of database abstraction would be in. Not that it is solving all issues, but it will help other to switch over to other DBs. Wicht mysqli this is not possible.

Next to this argument for PDO instead of mysqli, using mysqli prepare as referenced by you has implications:

A true prepared statement would be very bad, because it would do a return trip to the server for every query, and the server would actually prepare the statement ... (Ref. to Denis-de-Bernardy)

Hopefully this is usefull info for the ticket.

comment:11 in reply to: ↑ 10 Denis-de-Bernardy4 years ago

Replying to hakre:

A true prepared statement would be very bad, because it would do a return trip to the server for every query, and the server would actually prepare the statement ... (Ref. to Denis-de-Bernardy)

Yeah, I'd like to stress this point, because it's a bit subtle. A prepared statement will tell MySQL to come up with a query plan that fits for a given set of variables.

For instance, if you ask MySQL to prepare the following:

SELECT * FROM $wpdb->posts WHERE post_status = ? AND post_type = ? ORDER BY post_date LIMIT ?

The query planner would likely end up with a plan that fetches all rows and sorts the mess in memory (or worse, on the hard drive): the limit is an unknown, and it has no idea of how many rows might be returned due to the constraints on the type and status being unknowns as well.

Fill in the variables, however, and you get this:

SELECT * FROM $wpdb->posts WHERE post_status = 'publish' AND post_type = 'post' ORDER BY post_date LIMIT 10

In this case, the query planner can now take advantage of the table's stats, and knows that using an index on (post_status, post_type, post_date), for instance, will yield a very good plan.

When you use PDO::ATTR_EMULATE_PREPARES, the query no longer is prepared by MySQL. Rather, PDO sticks to using the table's row definitions and works out how to escape what based on them. The final query, without variables, is then sent to the query planner, which leads us to the second use-case above.

comment:12 in reply to: ↑ 10 ; follow-up: josephscott4 years ago

The hardest part of achieving database independence would not be helped at all by using PDO. From the PDO PHP intro page - http://us.php.net/manual/en/intro.pdo.php

PDO provides a data-access abstraction layer, which means that, regardless of which database you're using, you use the same functions to issue queries and fetch data. PDO does not provide a database abstraction; it doesn't rewrite SQL or emulate missing features. You should use a full-blown abstraction layer if you need that facility.

If you were really looking for a library to provide database independence there have been other options for quite some time.

The additional round trip requests to the DB to process the prepared statement would not be desirable though. I'd consider that enough to nix the idea of using real mysqli prepared statements.

I'm curious about the suggestion that the query planner would do poorly with prepared statements. Any there any references to this actually being the case?

comment:13 in reply to: ↑ 12 Denis-de-Bernardy4 years ago

Replying to josephscott:

I'm curious about the suggestion that the query planner would do poorly with prepared statements. Any there any references to this actually being the case?

I've seen it happen in my own apps, with queries very similar to my above example query.

Using PostgreSQL and with a decent amount of data, that example query ends up with a hash plan followed by a quick sort if you ask PG to prepare the statement, and with an index scan/nested loop plan if you send the final query instead. With large amounts of data, the difference between the two is significant.

The reason the optimizer behaves like that is the same as why query planers don't use indexes for small amounts of data: unless you need a small number of rows as compared to the total number of available rows, it's faster to open all disc pages and weed out the needed rows (using a hash bucket in this particular case), than it is to loop through an index.

comment:14 follow-up: josephscott4 years ago

I like PostgreSQL as much as the next person (probably more), but it's not really pertinent to the question. What does the MySQL query planner do? References and methods for reproducing would be helpful.

comment:15 in reply to: ↑ 14 Denis-de-Bernardy4 years ago

Replying to josephscott:

I like PostgreSQL as much as the next person (probably more), but it's not really pertinent to the question. What does the MySQL query planner do? References and methods for reproducing would be helpful.

I'd have happily posted the output of explain here if the functionality worked with anything but SELECT statements in MySQL.

There are only so many ways to optimize a query, though. If you find my own explanation lacking, then:

http://dev.mysql.com/doc/refman/5.1/en/mysql-indexes.html

Note that the first paragraph says exactly what I wrote in my previous reply:

If you need to access most of the rows, it is faster to read sequentially, because this minimizes disk seeks.

And then recall that the whole point of a prepared statement is to not need to parse the query and come up with a query plan before executing it. And no index can reliably be used if most of the constraints are variables.

comment:16 nacin4 years ago

Can we close this as wontfix? Rather than let this ticket rot for an undetermined number of releases while we still support PHP 4...

We can re-open when we get there. Clearly, when we go to PHP 5, we'll take a systematic approach which one would imagine includes considering the use of PDO. Surely we don't need this ticket in the meantime to remind us.

comment:17 dd324 years ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from new to closed
  • Version 2.9 deleted

I know I've said in the past theres nothing wrong with letting an enhancement sit in trac untouched for yonks (in preference to closing it). But in this case I'll make an exception.

When this ticket is technically possible to implement (ie. When we require a version of PHP5+ which always has the support loaded, or is loaded on 99% of hosts), feel free to re-open this ticket. Until such a time as its feasible to implement, I'd ask this ticket is left closed.

comment:18 mikemcquaid3 years ago

  • Cc mikemcquaid added
  • Resolution wontfix deleted
  • Status changed from closed to reopened

You require 5.2 now so I think this can be reopened?

comment:19 follow-up: scribu3 years ago

  • Keywords needs-patch dev-feedback removed
  • Resolution set to wontfix
  • Status changed from reopened to closed

Problems with using PDO:prepare():

  • requires a round-trip to the DB (this alone is a deal breaker)
  • is a PHP extension (i.e. not guaranteed to be installed)

I think it's safe to say that this will be wontfix for a long, long time.

comment:20 in reply to: ↑ 19 ; follow-up: Denis-de-Bernardy3 years ago

Replying to scribu:

Problems with using PDO:prepare():

  • requires a round-trip to the DB (this alone is a deal breaker)

Not when you emulate the prepares (PDO::ATTR_EMULATE_PREPARES, see http://php.net/manual/en/pdo.setattribute.php).

  • is a PHP extension (i.e. not guaranteed to be installed)

It is always installed in PHP 5.2

comment:21 scribu3 years ago

  • Milestone set to Awaiting Review
  • Resolution wontfix deleted
  • Status changed from closed to reopened

Thanks for the info.

comment:22 in reply to: ↑ 20 SergeyBiryukov3 years ago

Replying to Denis-de-Bernardy:

It is always installed in PHP 5.2

I thought the same about JSON. Turned out it's not, even if it should be.

comment:23 nacin3 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to maybelater
  • Status changed from reopened to closed

I think maybelater is a safe choice here. This kind of architectural decision is nowhere on the radar.

comment:24 magnus7817 months ago

  • Cc magnus.melin@… added

comment:25 mikemcquaid17 months ago

  • Cc mikemcquaid removed

comment:26 sirzooro17 months ago

Discussion is now continued in #21663.

Note: See TracTickets for help on using tickets.