New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Loads of DB queries to wpsc_get_meta() for country data #1894

Closed
benhuson opened this Issue May 29, 2015 · 16 comments

Comments

Projects
None yet
3 participants
@benhuson
Member

benhuson commented May 29, 2015

I'm seeing loads of DB queries to wpsc_get_meta() which look like it's to get region country info - about 250 queries!

SELECT `meta_value`
FROM `lmc_wp_wpsc_meta`
WHERE `object_type` = 'WPSC_Country'
AND `object_id` = 241
AND `meta_key` = 'region_label'

It's happening on every page so may be related to #1845

Haven't tracked down where it's coming from but it would seem that whatever is doing this could be done more efficiently as a single query for all country data.

This is on development for a production site using WPEC 3.9.3, theme engine v1.
I have not yet tested on trunk.

@benhuson

This comment has been minimized.

Show comment
Hide comment
@benhuson

benhuson May 29, 2015

Member

I can confirm it is happening on master branch.

It's the wpsc_get_meta() call in the WPSC_Country class (line 300) which is called 250 times making 250 separate SQL queries:

https://github.com/wp-e-commerce/WP-e-Commerce/blob/master/wpsc-includes/wpsc-country.class.php#L300

Member

benhuson commented May 29, 2015

I can confirm it is happening on master branch.

It's the wpsc_get_meta() call in the WPSC_Country class (line 300) which is called 250 times making 250 separate SQL queries:

https://github.com/wp-e-commerce/WP-e-Commerce/blob/master/wpsc-includes/wpsc-country.class.php#L300

@benhuson

This comment has been minimized.

Show comment
Hide comment
@benhuson

benhuson May 29, 2015

Member

Narrowed it down further to _wpsc_countries_localizations() being called in line 3 of checkout-localization.php to generate the country info for JavaScript in the header.

https://github.com/wp-e-commerce/WP-e-Commerce/blob/master/wpsc-components/theme-engine-v1/classes/checkout-localization.php#L3

Should this only be called on the checkout page?

It's the call in line 41 to $wpsc_country->get( 'region_label' ) in the loop that generates all the queries.
https://github.com/wp-e-commerce/WP-e-Commerce/blob/master/wpsc-components/theme-engine-v1/classes/checkout-localization.php#L41

Member

benhuson commented May 29, 2015

Narrowed it down further to _wpsc_countries_localizations() being called in line 3 of checkout-localization.php to generate the country info for JavaScript in the header.

https://github.com/wp-e-commerce/WP-e-Commerce/blob/master/wpsc-components/theme-engine-v1/classes/checkout-localization.php#L3

Should this only be called on the checkout page?

It's the call in line 41 to $wpsc_country->get( 'region_label' ) in the loop that generates all the queries.
https://github.com/wp-e-commerce/WP-e-Commerce/blob/master/wpsc-components/theme-engine-v1/classes/checkout-localization.php#L41

@JeffPyeBrook

This comment has been minimized.

Show comment
Hide comment
@JeffPyeBrook

JeffPyeBrook May 29, 2015

Contributor

Results are cached, so if your swing it on every page you have a cache issue

All the queries are eliminated in the PR I queue up

Contributor

JeffPyeBrook commented May 29, 2015

Results are cached, so if your swing it on every page you have a cache issue

All the queries are eliminated in the PR I queue up

@benhuson

This comment has been minimized.

Show comment
Hide comment
@benhuson

benhuson May 29, 2015

Member

Like the PR, will be good so see all that come out of the database.

I'm testing without any persistent object caching so it will be hit on each page I guess.

The weird thing is, I'm not even sure what "region_label" is?
It does't seem to exist anywhere in my db I don't think, and all 250 calls for it return no results.
I can't find any calls in core that set region_label meta, so what is it? is is used?

Member

benhuson commented May 29, 2015

Like the PR, will be good so see all that come out of the database.

I'm testing without any persistent object caching so it will be hit on each page I guess.

The weird thing is, I'm not even sure what "region_label" is?
It does't seem to exist anywhere in my db I don't think, and all 250 calls for it return no results.
I can't find any calls in core that set region_label meta, so what is it? is is used?

@JeffPyeBrook

This comment has been minimized.

Show comment
Hide comment
@JeffPyeBrook

JeffPyeBrook May 29, 2015

Contributor

Should still be caching in a transient?
On May 29, 2015 11:23 AM, "Ben Huson" notifications@github.com wrote:

Like the PR, will be good so see all that come out of the database.

I'm testing without any persistent object caching so it will be hit on
each page I guess.

The weird thing is, I'm not even sure what "region_label" is?

It does't seem to exist anywhere in my db I don't think, and all 250 calls
for it return no results.
I can't find any calls in core that set region_label meta, so what is it?
is is used?

Reply to this email directly or view it on GitHub
#1894 (comment)
.

Contributor

JeffPyeBrook commented May 29, 2015

Should still be caching in a transient?
On May 29, 2015 11:23 AM, "Ben Huson" notifications@github.com wrote:

Like the PR, will be good so see all that come out of the database.

I'm testing without any persistent object caching so it will be hit on
each page I guess.

The weird thing is, I'm not even sure what "region_label" is?

It does't seem to exist anywhere in my db I don't think, and all 250 calls
for it return no results.
I can't find any calls in core that set region_label meta, so what is it?
is is used?

Reply to this email directly or view it on GitHub
#1894 (comment)
.

@benhuson

This comment has been minimized.

Show comment
Hide comment
@benhuson

benhuson May 29, 2015

Member

Don't think it is. Is it just supposed to be storing that specific meta value as a transient, or the entire Country object?

The Query Monitor plugin is showing me 250 calls to wpsc_get_meta() on every page as per the example SQL above, each with a different object_id.

Also, taking a quick look at the wpsc_get_meta() function, it appears to use wp_cache_get() to try to get cache, but then does't do anything with it?
https://github.com/wp-e-commerce/WP-e-Commerce/blob/master/wpsc-includes/meta.functions.php#L15

Member

benhuson commented May 29, 2015

Don't think it is. Is it just supposed to be storing that specific meta value as a transient, or the entire Country object?

The Query Monitor plugin is showing me 250 calls to wpsc_get_meta() on every page as per the example SQL above, each with a different object_id.

Also, taking a quick look at the wpsc_get_meta() function, it appears to use wp_cache_get() to try to get cache, but then does't do anything with it?
https://github.com/wp-e-commerce/WP-e-Commerce/blob/master/wpsc-includes/meta.functions.php#L15

@JustinSainton

This comment has been minimized.

Show comment
Hide comment
@JustinSainton

JustinSainton May 29, 2015

Member

@benhuson There's an upgrade routine that sets "region_label" meta to States or Province, for US/CA respectively. See _wpsc_add_region_name_meta().

But yes, I'm not sure why we're using wpsc_get_meta at all in core - those functions have been broken for a long, long time.

Member

JustinSainton commented May 29, 2015

@benhuson There's an upgrade routine that sets "region_label" meta to States or Province, for US/CA respectively. See _wpsc_add_region_name_meta().

But yes, I'm not sure why we're using wpsc_get_meta at all in core - those functions have been broken for a long, long time.

@JustinSainton

This comment has been minimized.

Show comment
Hide comment
@JustinSainton

JustinSainton Jun 3, 2015

Member

@JeffPyeBrook @benhuson Open to suggestions here. Sadly, there are quite a number of instances throughout core (and who knows how many plugins) using this generic meta table and their APIs.

There are undocumented quirks here. For example - meta keys may only contain numbers, letters, and underscores. Good to know, right?

So - bottom line - these functions need to be unit tested and refactored to properly use the object cache layer, be well-documented, and generally less surprising.

Member

JustinSainton commented Jun 3, 2015

@JeffPyeBrook @benhuson Open to suggestions here. Sadly, there are quite a number of instances throughout core (and who knows how many plugins) using this generic meta table and their APIs.

There are undocumented quirks here. For example - meta keys may only contain numbers, letters, and underscores. Good to know, right?

So - bottom line - these functions need to be unit tested and refactored to properly use the object cache layer, be well-documented, and generally less surprising.

@JeffPyeBrook

This comment has been minimized.

Show comment
Hide comment
@JeffPyeBrook

JeffPyeBrook Jun 3, 2015

Contributor

Sadly this is one of the areas that had had a change queued up and was left as is.

The right thing to do is use the built in wordpress meta infrastructure to store this meta, pretty easy to do. The only wrinkle hear is that if all of the random meta is to be stored in one bucket the key needs to include the meta type. A saner approach would be to store each meta in its own table, and not invent a new mechanism. When we let WordPress do the work all the caching is handled without doing any extra work.

There is a template meta file here in the repository that can be used via a simple replace of text to create a new meta type. Then all you need is an upgrade routine to move the old meta type out of the big bucket into its own bucket. Take a look at the visitor meta implementation for an example.

Contributor

JeffPyeBrook commented Jun 3, 2015

Sadly this is one of the areas that had had a change queued up and was left as is.

The right thing to do is use the built in wordpress meta infrastructure to store this meta, pretty easy to do. The only wrinkle hear is that if all of the random meta is to be stored in one bucket the key needs to include the meta type. A saner approach would be to store each meta in its own table, and not invent a new mechanism. When we let WordPress do the work all the caching is handled without doing any extra work.

There is a template meta file here in the repository that can be used via a simple replace of text to create a new meta type. Then all you need is an upgrade routine to move the old meta type out of the big bucket into its own bucket. Take a look at the visitor meta implementation for an example.

@JustinSainton

This comment has been minimized.

Show comment
Hide comment
@JustinSainton

JustinSainton Jun 3, 2015

Member

@JeffPyeBrook I'm not super interested in adding another meta table for each of ~10 meta groups in core. I think the weight of adding an additional 10 meta tables far outweighs the benefits gained by inheriting some of WordPress's core meta functionality.

Member

JustinSainton commented Jun 3, 2015

@JeffPyeBrook I'm not super interested in adding another meta table for each of ~10 meta groups in core. I think the weight of adding an additional 10 meta tables far outweighs the benefits gained by inheriting some of WordPress's core meta functionality.

@JustinSainton JustinSainton added this to the 4.1 milestone Jun 3, 2015

@JeffPyeBrook

This comment has been minimized.

Show comment
Hide comment
@JeffPyeBrook

JeffPyeBrook Jun 3, 2015

Contributor

@justin as I noted above, the arbitrary meta values a single meta table can suffice, key can be created by combining the meta value key with the meta value type

Contributor

JeffPyeBrook commented Jun 3, 2015

@justin as I noted above, the arbitrary meta values a single meta table can suffice, key can be created by combining the meta value key with the meta value type

@JustinSainton

This comment has been minimized.

Show comment
Hide comment
@JustinSainton

JustinSainton Jun 3, 2015

Member

@JeffPyeBrook True - that'll take a fair degree of back compat work, since keys will be different - but not insurmountable.

Let's file a separate ticket for that - related: #400.

In the meantime - the core issue originally posted here is not yet resolved. @benhuson, any thoughts on that? PR totally welcomed.

Member

JustinSainton commented Jun 3, 2015

@JeffPyeBrook True - that'll take a fair degree of back compat work, since keys will be different - but not insurmountable.

Let's file a separate ticket for that - related: #400.

In the meantime - the core issue originally posted here is not yet resolved. @benhuson, any thoughts on that? PR totally welcomed.

@benhuson

This comment has been minimized.

Show comment
Hide comment
@benhuson

benhuson Jun 3, 2015

Member

I suspect that one approach would be to try to retrieve and cache all region meta in a single queries at the same time as all the countries are retrieved - like Posts and Post Meta?

I haven't delved deep enough into it yet to find the where that should happen.

Member

benhuson commented Jun 3, 2015

I suspect that one approach would be to try to retrieve and cache all region meta in a single queries at the same time as all the countries are retrieved - like Posts and Post Meta?

I haven't delved deep enough into it yet to find the where that should happen.

@benhuson

This comment has been minimized.

Show comment
Hide comment
@benhuson

benhuson Jun 16, 2015

Member

On an initial look into it, wpsc_get_meta() doesn't seem to use be using the WP_Object_Cache API properly, so prefetching meta to cache it (like how post meta works) is not going to work until that is fixed.

Essentially if you call $wpsc_country->get( 'region_label' ) multiple times on the same $wpsc_country object it does a DB request each time rather using wp_cache_get() and wp_cache_set() to save or return cached meta.

There is a use of wp_cache_get() in wpsc_get_meta() but it does't seem to use the returned value and it does't store it value is returned from the DB either.

I've had a quick search through the codebase for where wpsc_get_meta() is used and it seems to be primarily used for:

  • Category meta (e.g. target markets and sort order)
  • WPSC_Region
  • WPSC_Country
  • Sputnik?
  • wpsc-admin/includes/updating-functions.php

I presuming implementing the Object Cache API to work correctly should be fine in all these cases although I didn't delve into the admin update functions - i.e. use wp_cache_get() to try to get value and return if it exists, if not get it from the database, use wp_cache_set() to set it then return it.

@JeffPyeBrook do you have any thoughts on this?

Member

benhuson commented Jun 16, 2015

On an initial look into it, wpsc_get_meta() doesn't seem to use be using the WP_Object_Cache API properly, so prefetching meta to cache it (like how post meta works) is not going to work until that is fixed.

Essentially if you call $wpsc_country->get( 'region_label' ) multiple times on the same $wpsc_country object it does a DB request each time rather using wp_cache_get() and wp_cache_set() to save or return cached meta.

There is a use of wp_cache_get() in wpsc_get_meta() but it does't seem to use the returned value and it does't store it value is returned from the DB either.

I've had a quick search through the codebase for where wpsc_get_meta() is used and it seems to be primarily used for:

  • Category meta (e.g. target markets and sort order)
  • WPSC_Region
  • WPSC_Country
  • Sputnik?
  • wpsc-admin/includes/updating-functions.php

I presuming implementing the Object Cache API to work correctly should be fine in all these cases although I didn't delve into the admin update functions - i.e. use wp_cache_get() to try to get value and return if it exists, if not get it from the database, use wp_cache_set() to set it then return it.

@JeffPyeBrook do you have any thoughts on this?

@benhuson

This comment has been minimized.

Show comment
Hide comment
@benhuson

benhuson Jun 17, 2015

Member

PR #1908 doesn't entirely fix this, just paves the way for it by ensuring wpsc_get_meta() results are cached the first time they are called for a specific object ID, so multiple calls for the same object meta don't generate multiple DB requests.

At the moment wpsc_get_meta() is still called 250 times to get all the different country region label meta - so 250 DB queries.

Now that it correctly checks cache first it should be possible to write a query to get meta for all countries up front and cache it so that any subsequent calls to wpsc_get_meta() to get individual meta don;t results in extra DB hits - similar to the way post meta it pre-fetched.

Member

benhuson commented Jun 17, 2015

PR #1908 doesn't entirely fix this, just paves the way for it by ensuring wpsc_get_meta() results are cached the first time they are called for a specific object ID, so multiple calls for the same object meta don't generate multiple DB requests.

At the moment wpsc_get_meta() is still called 250 times to get all the different country region label meta - so 250 DB queries.

Now that it correctly checks cache first it should be possible to write a query to get meta for all countries up front and cache it so that any subsequent calls to wpsc_get_meta() to get individual meta don;t results in extra DB hits - similar to the way post meta it pre-fetched.

@benhuson benhuson reopened this Jun 17, 2015

@JustinSainton

This comment has been minimized.

Show comment
Hide comment
@JustinSainton

JustinSainton Jun 17, 2015

Member

sorry, got a little trigger happy :P

Member

JustinSainton commented Jun 17, 2015

sorry, got a little trigger happy :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment