WordPress.org

Make WordPress Core

Opened 10 years ago

Closed 9 years ago

Last modified 9 years ago

#33 closed defect (bug) (fixed)

Three sql queries with where clauses that can only contain 1=1

Reported by: joseph Owned by: matt
Milestone: Priority: normal
Severity: trivial Version:
Component: General Keywords:
Focuses: Cc:

Description

There are three sql queries in the current cvs code with where clauses that can only ever contain 1=1. There is a another query that has 1=1 in the where clause, but it looks like it may be there because other where options may be optional. If it turns out that this is not the case then it too can be removed.

Attachments (1)

0000033-wp.diff (2.2 KB) - added by joseph 9 years ago.

Download all attachments as: .zip

Change History (8)

comment:2 hadj10 years ago

As is typical of open source AMP projects, some of the code in WordPress is a hack. The good news: Our code is far, far (far, far, far) cleaner than say PHP-Nuke, because we use best practices like CSS, and object classes like the wpdb object. The bad news - too often, our database queries are as sloppy / hackish as the worst of Nuke. We are still at v 1.x, Nuke is on v 7.x...by the time we have our own v. 7 code, it will be nearly as insrutable as Nuke's is, unless we do a better job at standardizing than they did. The place to start is with database queries:

# Don't write queries that return trivial or irrelevant results. This bug is an example. An even better one is found in wp-admin/post.php, v 1.2, line X. When you've finished inserting a row into $tableposts, the ID on that row is guaranteed unique (GUID). So there is no need to test $tablepost2cat for *each* category to see if that row already exists in $tablepost2cat -- you could just do *one* "select ref_ID from $tablepost2cat where post_id = $post_id" and iff you get any rows back you need to throw an error. Better still, $tablepost2cat should have a UNIQUE index on post_id, cat_id; in v 1.2 it only has a non-unique INDEX. Better than that, MySQL lets you define table relationships so that no row can be added to $tablepost2cat unless its post_id matchs an ID already present in $tableposts.

# Learn the features of the wpdb object, and USE THEM. In the above example, with the table relationships enforced by the RDBMS rather than by the php code, the under-appreciated wpdb object sets the proper error condition if you try to insert a second row with the same post_id and cat_id into $tablepost2cat. This way, the entire insert to $tablepost2cat fails, not just that one row, leaving that table in a consistant state. Once you understand the properties and functions avaiable from the wpdb object, you can easily trap the error and display a sarcastic "gee, you REEEELLY like categories, don't you?". Or you could just let the page fail with a runtime error, since whoever is causing this error is up to no good. For example, they might have submited a bogus HTTP POST indicating a blog post is a member of category X more than one time. This hack would be passed to the global $_POSTScategories? which would return an array with more than one key having the same value.

# After an INSERT, you *don't* need to do another SELECT to fetch the primary key (autoincrement) value for that row. If your code does something like $result = $wpdb->query("INSERT whatever");, in the next line you can write $id = $result->last_insert_ID and presto you have just elminated an entire round-trip to the DB! I think my pseudo-code is accurate, if it's not, the details are in wp-includes/wp-db.php, search for last_insert in that file.

# Simplify your WHERE clause. Or, if a complex where condtion is needed to cover some subtle special cases, comment your code to explain why you wrote the clause the way you did. This bug is an example of a problem that is unfortunately too wide-spread in WordPress.

# Don't run SELECT * when all you need is a single column, or a list of small columns. Remember, even if your WHERE clause is efficient, it still takes work for the database to fetch the value of all columns for a given row. Worse, the PHP <-> MySQL api / middleware has to incurr a lot more overhead to define a rowset with many columns of varying data types and fill each column with data. Why make your server(s) thrash to bring you all 1K - 100++K of a large post, when all you need is the post_ID and maybe the author_id or some other flags?

SELECT * is a shorthand created for use in ad-hoc querys so your fingers don't get tired. Don't use it for production level code!

However, you can freely use SELECT COUNT(*) without fear. MySQL is smart enough to treat this as the equivalent to the much more precise SELECT COUNT(primarykey). And of course, you know that if you want the COUNT of rows in a column C in which nulls are allowed, you have to do COUNT(C) to avoid including the null rows in your count :-)


At some point, probably when we go to WP 2.0, there will be a slew of unavoidable changes to the DB structure (added tables, added columns to existing tables, in order to support new or enhanced features; depreciated tables or columns removed, etc.) Right now is the time to start incrementally improving the php code, table structure, constraints, best practices, etc. so that after the upgrade we will have cleaner code and DB, plus all those nifty features. Toodles.

comment:3 hadj10 years ago

Reminder sent to 2fargon, Anne, joseph, matt, rboren, skippy

I just added a bugnote to bug 33 that appeals for better coding / db practices in the future. Let me know what you think. Are these sorts of changes already in progress? I could probably pitch in to help over the winter...

comment:4 2fargon9 years ago

  • Patch set to Yes
  • Status changed from new to assigned

Joseph, thanks for the report. We will look into optimization once the feature additions and other actions are complete. Thank you for the patience, things often mover slower than one would expect. Please do not think that this is because no one cares, or that the bug has been overlooked. Look forward to your help in the future, too.

comment:5 matt9 years ago

PLEASE *do not* SPAM developers from the bug tracker, no matter how important you think the issue is.

edited on: 01-11-05 01:17

comment:6 matt9 years ago

  • fixed_in_version set to 1.5
  • Resolution changed from 10 to 20
  • Status changed from assigned to closed

comment:7 matt9 years ago

  • Owner changed from anonymous to matt

joseph9 years ago

Note: See TracTickets for help on using tickets.