WordPress.org

Make WordPress Core

Opened 9 years ago

Closed 6 years ago

Last modified 6 years ago

#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:
PR Number:

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 9 years ago.
15004.2.diff (456 bytes) - added by nacin 7 years ago.
15004.3.diff (488 bytes) - added by pento 7 years ago.
15004.4.diff (551 bytes) - added by pento 7 years ago.
15004.5.diff (515 bytes) - added by pento 7 years ago.
15004.6.diff (848 bytes) - added by josephscott 6 years ago.
Add primary key as well
wp-signups.php (2.9 KB) - added by josephscott 6 years ago.
Test script on the impact of changing indexes
15004.7.diff (1.8 KB) - added by josephscott 6 years ago.
now includes ALTER TABLE actions
15004.8.diff (1.5 KB) - added by josephscott 6 years ago.
Move to pre_schema_upgrade

Download all attachments as: .zip

Change History (36)

@barry
9 years ago

#1 @barry
9 years ago

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

#2 @nacin
9 years ago

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

#3 @SergeyBiryukov
8 years ago

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

#4 in reply to: ↑ description @nacin
8 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
8 years ago

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

#6 @ryan
8 years ago

  • Milestone changed from 3.3 to Future Release

#7 @nacin
7 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
7 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
7 years ago

#9 @nacin
7 years ago

  • Description modified (diff)

#10 @nacin
7 years ago

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

@pento
7 years ago

@pento
7 years ago

#11 @pento
7 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
7 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
7 years ago

#13 @pento
7 years ago

  • Keywords needs-refresh removed

Agreed, fixed in patch 5.

#14 @nacin
7 years ago

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

#15 follow-up: @nacin
7 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
7 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
7 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
7 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
7 years ago

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

@josephscott
6 years ago

Add primary key as well

#20 @josephscott
6 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
6 years ago

Test script on the impact of changing indexes

#21 @josephscott
6 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
6 years ago

now includes ALTER TABLE actions

#22 @josephscott
6 years ago

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

@josephscott
6 years ago

Move to pre_schema_upgrade

#23 @josephscott
6 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
6 years ago

In 25179:

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

props josephscott, pento, barry.
see #15004.

#25 @nacin
6 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
6 years ago

  • Cc kovshenin added

#27 @nacin
6 years ago

In 25413:

Fix comma placement in [25179].

props gradyetc.
see #15004, fixes #25298.

Note: See TracTickets for help on using tickets.