Skip to content
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

Fixes #7049: Performance issue with table existence query #7050

Merged

Conversation

jeawhanlee
Copy link
Contributor

Description

This PR fixes the issue with user servers being impacted due to unoptimized query.

Fixes #7049

Type of change

  • Bug fix (non-breaking change which fixes an issue).

Detailed scenario

Not able to reproduce on local at the moment.

Technical description

Documentation

The query that checks if table exists is now optimized to specifically target the current DB instead of recurrently checking tables in all the Databases on the server.

New dependencies

N/A

Risks

N/A

Mandatory Checklist

Code validation

  • I validated all the Acceptance Criteria. If possible, provide screenshots or videos.
  • I triggered all changed lines of code at least once without new errors/warnings/notices.

Code style

  • I wrote a self-explanatory code about what it does.
  • I did not introduce unnecessary complexity.

@jeawhanlee jeawhanlee self-assigned this Oct 22, 2024
Copy link

codacy-production bot commented Oct 22, 2024

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
Report missing for 815e6391 100.00% (target: 50.00%)
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (815e639) Report Missing Report Missing Report Missing
Head commit (79a26d3) 37750 16597 43.97%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#7050) 4 4 100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

Footnotes

  1. Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.

@jeawhanlee jeawhanlee added the type: bug Indicates an unexpected problem or unintended behavior label Oct 22, 2024
@jeawhanlee jeawhanlee requested a review from a team October 22, 2024 11:40
@jeawhanlee jeawhanlee marked this pull request as ready for review October 22, 2024 11:40
Copy link
Contributor

@Miraeld Miraeld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is this one to change as well

protected function table_exists(): bool {
if ( self::$table_exists ) {
return true;
}
// Get the database interface.
$db = $this->get_db();
// Bail if no database interface is available.
if ( ! $db ) {
return false;
}
// Query statement.
$query = 'SELECT table_name FROM information_schema.tables WHERE table_name = %s LIMIT 1';
$prepared = $db->prepare( $query, $db->{$this->table_name} );
$result = $db->get_var( $prepared );
// Does the table exist?
$exists = $this->is_success( $result );
if ( $exists ) {
self::$table_exists = $exists;
}
return $exists;
}

@jeawhanlee jeawhanlee requested a review from Miraeld October 22, 2024 14:00
Copy link
Contributor

@remyperona remyperona left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For later: we should see how to avoid having duplicated code for this

@Mai-Saad
Copy link
Contributor

Test plan:

@Miraeld
Copy link
Contributor

Miraeld commented Oct 28, 2024

Hello,
NRT is green :)
testrail-report-652.pdf

@Miraeld
Copy link
Contributor

Miraeld commented Oct 29, 2024

Hello hello,

I've been trying to reproduce the issue in a local environment.
To do this, I made a Docker environment with 2 main containers: 1 Wordpress, and 1 DB server.

On this DB server, I've created 10 000 databases where each one of them has their own user and these aren't SUPER user and of course, there is a single SUPER User that has access to everything.

So to make it short.

10 000 databases, they all have the same named tables. So in my example I'll use table_1.

There is 1 SUPER User, otherwise, each database have a different user which isn't SUPER.

From the SUPER User:
The query: SELECT table_name FROM information_schema.tables; returns all tables in every database, which means if I do
SELECT table_name FROM information_schema.tables WHERE table_name = 'table_1'; I'll get 10k rows results.

Now, from a normal user from 1 db,
If I run SELECT table_name FROM information_schema.tables I only get the table of its own database.

and SELECT table_name FROM information_schema.tables WHERE table_name = 'table_1' LIMIT 1; (similar to what we had in WP Rocket) runs within 0.017s

While the new query introduced by #7049
is SELECT table_name FROM information_schema.tables WHERE table_schema = 'wordpress' AND table_name = 'table_1' LIMIT 1 runs within 0.002s.

So we can see an improvement, as we are reducing the execution time of the query by almost 10.

@wordpressfan wordpressfan added this pull request to the merge queue Oct 29, 2024
Merged via the queue into develop with commit 9f3b827 Oct 29, 2024
13 checks passed
@wordpressfan wordpressfan deleted the fix/7049-performance-issue-with-table-existence-query branch October 29, 2024 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Performance issue with table existence query
5 participants