Make WordPress Core

Opened 13 years ago

Closed 11 years ago

Last modified 11 years ago

#15004 closed defect (bug) (fixed)

Missing index on signups table

Reported by: barry's profile barry Owned by: pento's profile 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 nacin)

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)

15004.diff (457 bytes) - added by barry 13 years ago.
15004.2.diff (456 bytes) - added by nacin 12 years ago.
15004.3.diff (488 bytes) - added by pento 12 years ago.
15004.4.diff (551 bytes) - added by pento 12 years ago.
15004.5.diff (515 bytes) - added by pento 12 years ago.
15004.6.diff (848 bytes) - added by josephscott 11 years ago.
Add primary key as well
wp-signups.php (2.9 KB) - added by josephscott 11 years ago.
Test script on the impact of changing indexes
15004.7.diff (1.8 KB) - added by josephscott 11 years ago.
now includes ALTER TABLE actions
15004.8.diff (1.5 KB) - added by josephscott 11 years ago.
Move to pre_schema_upgrade

Download all attachments as: .zip

Change History (36)

@barry
13 years ago

#1 @barry
13 years ago

  • Summary changed from Missing index on wp_signups to Missing index on signups table

#2 @nacin
13 years ago

  • Keywords 3.2-early commit added
  • Milestone changed from Awaiting Review to Future Release

#3 @SergeyBiryukov
13 years ago

  • Keywords 3.2-early removed
  • Milestone changed from Future Release to 3.3

#4 in reply to: ↑ description @nacin
13 years ago

Replying to barry:

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.

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.

#5 @ryan
13 years ago

Seems safe. It bothers me too that they are flipped.

#6 @ryan
12 years ago

  • Milestone changed from 3.3 to Future Release

#7 @nacin
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 @nacin
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.

@nacin
12 years ago

#9 @nacin
12 years ago

  • Description modified (diff)

#10 @nacin
12 years ago

user_login also has a query running against it, also in wpmu_validate_user_signup(). Both fields should be indexed.

@pento
12 years ago

@pento
12 years ago

#11 @pento
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 @nacin
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.

@pento
12 years ago

#13 @pento
12 years ago

  • Keywords needs-refresh removed

Agreed, fixed in patch 5.

#14 @nacin
12 years ago

I think we need to remove the domain key via a separate ALTER in the upgrade routine.

#15 follow-up: @nacin
11 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 @nacin
11 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 @nacin
11 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 @nacin
11 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.

#19 @nacin
11 years ago

Closed as a duplicate — #24279, for adding a primary key here.

@josephscott
11 years ago

Add primary key as well

#20 @josephscott
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.

@josephscott
11 years ago

Test script on the impact of changing indexes

#21 @josephscott
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.

@josephscott
11 years ago

now includes ALTER TABLE actions

#22 @josephscott
11 years ago

New diff - 15004.7.diff - that includes the ALTER TABLE actions needed when doing a network update.

@josephscott
11 years ago

Move to pre_schema_upgrade

#23 @josephscott
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.

#24 @nacin
11 years ago

In 25179:

Add signup_id primary key to $wpdb->signups, and add better indexes.

props josephscott, pento, barry.
see #15004.

#25 @nacin
11 years ago

  • Milestone changed from Future Release to 3.7
  • Resolution set to fixed
  • Status changed from assigned to closed

#25192 for leveraging signup_id in queries.

#26 @kovshenin
11 years ago

  • Cc kovshenin added

#27 @nacin
11 years ago

In 25413:

Fix comma placement in [25179].

props gradyetc.
see #15004, fixes #25298.

Note: See TracTickets for help on using tickets.