Make WordPress Core

Opened 6 months ago

Last modified 5 weeks ago

#60299 assigned defect (bug)

Default assignment of `use_ssl` user meta value causes unnecessary DB write.

Reported by: prettyboymp's profile prettyboymp Owned by: adamsilverstein's profile adamsilverstein
Milestone: 6.7 Priority: normal
Severity: normal Version:
Component: Users Keywords: has-patch has-unit-tests has-testing-info needs-testing
Focuses: performance Cc:

Description

When the $meta in wp_insert_user() is processed using update_metadata() it first does a comparison against the current values in the database prior to running an update query. However, because the DB returns a string value ('1' or '0') while the meta_value is set to a boolean value by default the comparison always fails causing a write when not needed.

While this is relatively minor in itself, in multi-datacenter setups using hyperdb, this can cause a bit of overhead since all future reads to the usermeta table are then sent to the master database.

Attachments (1)

60299.patch (695 bytes) - added by prettyboymp 6 months ago.
Set $meta['use_ssl'] to string value.

Download all attachments as: .zip

Change History (20)

@prettyboymp
6 months ago

Set $meta['use_ssl'] to string value.

#1 @prettyboymp
6 months ago

  • Keywords has-patch added

#2 @SergeyBiryukov
6 months ago

  • Milestone changed from Awaiting Review to 6.5

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


5 months ago

#4 @swissspidy
5 months ago

  • Keywords needs-unit-tests added

#5 @swissspidy
5 months ago

  • Milestone changed from 6.5 to 6.6

This ticket was mentioned in PR #6422 on WordPress/wordpress-develop by @rajinsharwar.


3 months ago
#6

  • Keywords has-unit-tests added; needs-unit-tests removed

Removing unnecessary writes to the DB for use_ssl user meta, with unit tests.

Trac ticket: https://core.trac.wordpress.org/ticket/60299

#7 @rajinsharwar
3 months ago

  • Keywords needs-testing has-testing-info added

Added the unit tests for this.

Instructions to test if someone is willing to help:

  1. Edit any user from the Admin Dashboard, and copy that user's ID.
  1. Add the below lines just after the statement of function update_metadata( $meta_type, $object_id, ..)
	if ( $meta_key == 'use_ssl' ){
		error_log( 'Use SSL value in Meta: ' . ' ' . print_r(gettype($meta_value), true));
	}
  1. Now, in your theme's functions.php, add the below line of code. Replace the USER_ID with the int value you copied in step 1.
error_log( 'Use SSL value in DB: ' . ' ' . gettype(get_user_meta( USER_ID, 'use_ssl', true )) );
  1. Without making any changes to the Profile of that user, simply click "Update User"
  1. In your Error Log file, you should see that both the datatypes in the error log outputs will be different. ❌
  1. Now, apply the patch, and re-do the steps. You will find the Data types are the same. ✅

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


3 months ago

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


2 months ago

@johnbillion commented on PR #6422:


2 months ago
#10

@Rajinsharwar Many thanks for the PR. Are you able to take a look at the feedback above from Adam?

@rajinsharwar commented on PR #6422:


2 months ago
#11

Hi @johnbillion, I have updated the PR with @adamsilverstein feedback. :)

#12 @rajinsharwar
2 months ago

  • Keywords commit added; needs-testing removed

I believe this ticket doesn't need much testing, as it's a fairly small change itself. Removing 'needs-testing', and adding 'commit'.

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


7 weeks ago

#14 @adamsilverstein
7 weeks ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

#15 @adamsilverstein
7 weeks ago

  • Keywords needs-testing added; commit removed

Thanks for updating the PR with tests @rajinsharwar and for providing testing instructions. I'll try to get this tested and then committed for 6.6.

#16 @rayhatron
6 weeks ago

As part of WCEU contributor day, I'm picking up the testing of this ticket.

#17 @rayhatron
5 weeks ago

My notes from looking at this at WCEU contributor day:

wp_insert_user() receives the userdata from wp_update_user() here. wp_update_user() receives the userdata from edit_user() here. In edit_user(), we see use_ssl being set to either 0 or 1 as integers here.

Coming back to wp_insert_user, we see here $meta['use_ssl'] being set to either the integer 0 if $userdata['use_ssl'] is empty otherwise it sets it to the boolean equivalent of what $userdata['use_ssl'] would have.

In the case where we have the userdata being passed as explained earlier, it means $meta['use_ssl'] will either be true or false. So when update_user_meta() is called and it calls update_metadata(), we then see the old value already stored in DB (which is a string, see schema) being compared to the boolean in $meta['use_ssl'] here.

And those two values won't match the strict comparison since they're different values and data types. This then means $wpdb->update() is called here. This is the unnecessary db write that was mentioned in the description.

Now the patch did 2 things:

  1. Set $meta['use_ssl'] to the string '0' instead of the integer 0 if $userdata['use_ssl'] is empty.
  2. Force set $meta['use_ssl'] to the string '1' instead of attempting to retrieve it from the passed $userdata['use_ssl'].

We have a filter insert_user_meta and in its documentation $use_ssl is specified to either be an int or bool. This means changing $meta['use_ssl'] to be a string will potentially cause existing code in the community that hooks on that filter to break because the datatype has changed from what was originally specified.

So a different approach that we can take is at the point of comparing, if the meta key is use_ssl then we make sure the old value retrieved from the database is changed to the same datatype as the new value which will either be an int or a boolean.

We'll also need to make the unit tests verify that we indeed have 1 less db query.

Last edited 5 weeks ago by rayhatron (previous) (diff)

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


5 weeks ago

#19 @mukesh27
5 weeks ago

  • Milestone changed from 6.6 to 6.7

Hi there!

We discussed this ticket in today's performance bug scrub. The findings in https://core.trac.wordpress.org/ticket/60299#comment:17 seem valid, so we need more eyes on it.

Punted to the next milestone. Feel free to add it to the milestone if it's ready for commit.

additional props: @joemcgill

Note: See TracTickets for help on using tickets.