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

int (ID) is enclosed in quotes when using WHERE #230

Closed
patriksh opened this issue May 4, 2020 · 10 comments
Closed

int (ID) is enclosed in quotes when using WHERE #230

patriksh opened this issue May 4, 2020 · 10 comments

Comments

@patriksh
Copy link
Contributor

patriksh commented May 4, 2020

$this->dao->where('t.i_thread_id', $thread_id');

results in:

WHERE t.i_thread_id = '22745'

$this->dao->where("t.i_thread_id = $thread_id");

results in:

WHERE t.i_thread_id = 22745

This hurts performance and doesn't allow indexes to function well. We should check if where argument is a number and remove quotes in that case.

@patriksh patriksh added the bug label May 4, 2020
@patriksh patriksh changed the title IDs is enclosed in quotes when using WHERE int (ID) is enclosed in quotes when using WHERE May 4, 2020
@patriksh
Copy link
Contributor Author

patriksh commented May 4, 2020

Running a few queries resulted in an overall 16% of query time decrease.
These are very small numbers tho, but on bigger sites, databases, and queries it could help.

Note that i_thread_id is indexed.

@dev-101
Copy link
Contributor

dev-101 commented May 4, 2020

There's also this optimization here: osclass/Osclass#2293
However, I couldn't rewrite model side alone.

@patriksh
Copy link
Contributor Author

patriksh commented May 4, 2020

There's also this optimization here: osclass/Osclass#2293
However, I couldn't rewrite model side alone.

No changes have been made yet regarding that?

I think just the INDEX they mentioned would help a lot. I'm working on optimizing some plugins on a large site and just by adding some good indexes the speed of queries was increased significantly.

@dev-101
Copy link
Contributor

dev-101 commented May 4, 2020

Nope. But, optimized query is not exactly the same, so adding index alone isn't good enough IIRC.

@patriksh
Copy link
Contributor Author

patriksh commented May 4, 2020

Nope. But, optimized query is not exactly the same, so adding index alone isn't good enough IIRC.

Agree, but until we start working with optimizing the DAO classes, index is very easy to add.

@dev-101
Copy link
Contributor

dev-101 commented May 4, 2020

Yes, that part is easy :)

@navjottomer
Copy link
Member

@webmods-croatia It would be better to compare it against the allowed characters.
ASCII: [0-9,a-z,A-Z$_] (basic Latin letters, digits 0-9, dollar, underscore)

BTW I tried to run both queries with EXPLAIN and I didn't found any performance-related issue due to quotes nor I find any documentation related to performance impact.

EXPLAIN Select * from oc_t_item where fk_i_category_id = '31'
|id |select_type|table    |partitions|type|possible_keys   |key             |key_len|ref  |rows|filtered|Extra|
|---|-----------|---------|----------|----|----------------|----------------|-------|-----|----|--------|-----|
|1  |SIMPLE     |oc_t_item|NULL      |ref |fk_i_category_id|fk_i_category_id|4      |const|59  |100.00  |NULL |
EXPLAIN Select * from oc_t_item where fk_i_category_id = 31
|id |select_type|table    |partitions|type|possible_keys   |key             |key_len|ref  |rows|filtered|Extra|
|---|-----------|---------|----------|----|----------------|----------------|-------|-----|----|--------|-----|
|1  |SIMPLE     |oc_t_item|NULL      |ref |fk_i_category_id|fk_i_category_id|4      |const|59  |100.00  |NULL |

As you can see in above case both queries are using indexes.

@patriksh
Copy link
Contributor Author

patriksh commented May 7, 2020

@navjottomer I ain't an expert with this, but that's what EverSQL told me. The performance difference is there.

@navjottomer
Copy link
Member

Can you share the report? If the performance difference is there then we should see the whole example.

@navjottomer
Copy link
Member

Closing this for now.

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

No branches or pull requests

3 participants