#15004 closed defect (bug) (fixed)
Missing index on signups table
Reported by: | barry | Owned by: | pento |
---|---|---|---|
Milestone: | 3.7 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Database | Keywords: | has-patch commit needs-refresh 3.6-early |
Focuses: | Cc: |
Description (last modified by )
wp-includes/ms-functions.php:590: $signup = $wpdb->get_row( $wpdb->prepare("SELECT * FROM $wpdb->signups WHERE user_email = %s", $user_email) ); wp-includes/ms-functions.php:595: $wpdb->query( $wpdb->prepare("DELETE FROM $wpdb->signups WHERE user_email = %s", $user_email) );
But there is no index on user_email in the signups table. Makes these queries perform a full table scan which is slow when you have lots of signups.
Attached patch adds the index but I can't figure out how schema upgrades on MS-specific tables ever get run after the initial activation of MS mode and table creation.
Attachments (9)
Change History (36)
#1
@
14 years ago
- Summary changed from Missing index on wp_signups to Missing index on signups table
#2
@
14 years ago
- Keywords 3.2-early commit added
- Milestone changed from Awaiting Review to Future Release
#4
in reply to:
↑ description
@
13 years ago
#7
@
12 years ago
- Milestone changed from Future Release to 3.5
// Upgrade global tables only for the main site. Don't upgrade at all if DO_NOT_UPGRADE_GLOBAL_TABLES is defined. if ( in_array( $table, $global_tables ) && ( !is_main_site() || defined( 'DO_NOT_UPGRADE_GLOBAL_TABLES' ) ) ) continue;
So, dbDelta() is already aware that we should avoid upgrading global tables except when operating on the main site (and the danger flag isn't set). It also looks like make_db_current_silent() will trigger an upgrade for wp_get_db_schema( 'all' ), which includes global tables. Haven't tested, but it appears schema changes to global tables will now get picked up just fine. All we'd need is a refresh of 15004.diff and a DB version bump.
#8
@
12 years ago
- Keywords has-patch added
Before:
id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra |
1 | SIMPLE | wp31beta_signups | ALL | 30 | Using where |
After:
id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra |
1 | SIMPLE | wp31beta_signups | ref | user_email | user_email | 302 | const | 1 | Using where |
(Separately, you know what's fun? No primary key on the signups table.)
New patch properly names the key and refreshes it against trunk.
#10
@
12 years ago
user_login also has a query running against it, also in wpmu_validate_user_signup(). Both fields should be indexed.
#11
@
12 years ago
(user_login,user_email)
is a reasonable PK. I also noticed a query that was deleting by (domain,path)
, so swapping the domain
key for domain_path
will improve that.
#12
@
12 years ago
- Keywords needs-refresh added
- Owner set to pento
- Status changed from new to assigned
As adding a PK will fail if we have duplicate rows (and, well, who knows), let's go with a user_login,user_email key. Looks good otherwise.
#14
@
12 years ago
I think we need to remove the domain
key via a separate ALTER in the upgrade routine.
#15
follow-up:
↓ 16
@
12 years ago
Removal of the domain
key would happen in pre_schema_upgrade()
, if is_multisite() && is_main_site() && ! defined( 'DO_NOT_UPGRADE_GLOBAL_TABLES' )
.
We need a way to not fire the DROP INDEX for this for every main site of a multi-network. There's no harm in running a DROP INDEX on an index that doesn't exist (we will need to suppress errors) or we can first check if the index is there before dropping it.
We can do this prior to beta 2.
#16
in reply to:
↑ 15
@
12 years ago
Replying to nacin:
We can do this prior to beta 2.
If ryan doesn't mind. It's a bit unorthodox for us to do schema changes anytime but super early in the cycle.
#17
@
12 years ago
- Keywords needs-refresh added
pento, in IRC: "I'd be inclined to do a SHOW INDEX. Suppressing errors would be bad if there was an error we cared about."
#18
@
12 years ago
- Keywords 3.6-early added
- Milestone changed from 3.5 to Future Release
We long passed the appropriate time for a schema change.
#20
@
11 years ago
Added 15004.6.diff which takes the new KEYs from 15004.5.diff and adds a new signup_id column that is used as a primary key.
#21
@
11 years ago
@nacin asked about the impact of adding a signup_id column on a wp_signups table that already had 100,000 rows in it. I put together a simple test script to build a signups table, fill it with 100,000 records, add the signup_id column as a primary key, drop the domain key, and add the other suggested keys. The script is attached to this ticket - http://core.trac.wordpress.org/attachment/ticket/15004/wp-signups.php
On my Macbook Air the impact of the key changes was fairly small:
time php wp-signups.php - Creating j5test_signups table ... 0.02 seconds - Adding 100,000 rows to j5test_signups ... 43.73 seconds - Adding signup_id column ( PK ) to j5test_signups ... 1.67 seconds - Dropping key on domain column ... 0.04 seconds - Adding key on domain,path ... 0.41 seconds - Adding key on user_login,user_email ... 0.49 seconds - Adding key on user_email ... 0.37 seconds - Dropping j5test_signups ... 0.04 seconds real 0m46.996s user 0m16.814s sys 0m1.434s
Adding the signup_id column and all of the other key changes took roughly 3 seconds.
#22
@
11 years ago
New diff - 15004.7.diff - that includes the ALTER TABLE actions needed when doing a network update.
#23
@
11 years ago
New 15004.8.diff that does a few things:
- lets dbDelta take care of adding the new indexes
- moves adding the signup_id PK to pre_schema_upgrade() to avoid errors with dbDelta
- moves DROPing the domain index to pre_schema_ugprade()
No errors at this point from dbDelta and all of the wp_signups table changes are working.
Replying to barry:
They don't. We'll need to call install_network() in upgrade.php. Possibly only in an upgrade_XYZ() function rather than on every upgrade.
Ryan, would it be safe to move install_network() to schema.php and populate_network() to upgrade.php? It bothers me that those functions are in the opposite files.