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

Add index on Channel::hostname to prevent table scan on each request #9081

Merged
merged 1 commit into from
Jan 2, 2018

Conversation

stefandoorn
Copy link
Contributor

Q A
Branch? 1.1 / master
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Related tickets replaced #9080
License MIT

The first query occuring on each request in my app does a call to the ChannelRepository::findOneByHostname method from the HostnameBasedRequestResolver::findChannel method, resulting in a query looking like this:

EXPLAIN SELECT t0.code AS code_1, t0.name AS name_2, t0.color AS color_3, t0.description AS description_4, t0.enabled AS enabled_5, t0.hostname AS hostname_6, t0.created_at AS created_at_7, t0.updated_at AS updated_at_8, t0.id AS id_9, t0.theme_name AS theme_name_10, t0.tax_calculation_strategy AS tax_calculation_strategy_11, t0.contact_email AS contact_email_12, t0.skipping_shipping_step_allowed AS skipping_shipping_step_allowed_13, t0.skipping_payment_step_allowed AS skipping_payment_step_allowed_14, t0.account_verification_required AS account_verification_required_15, t0.default_locale_id AS default_locale_id_16, t17.code AS code_18, t17.created_at AS created_at_19, t17.updated_at AS updated_at_20, t17.id AS id_21, t0.base_currency_id AS base_currency_id_22, t23.code AS code_24, t23.created_at AS created_at_25, t23.updated_at AS updated_at_26, t23.id AS id_27, t0.default_tax_zone_id AS default_tax_zone_id_28 FROM sylius_channel t0 INNER JOIN sylius_locale t17 ON t0.default_locale_id = t17.id INNER JOIN sylius_currency t23 ON t0.base_currency_id = t23.id WHERE t0.hostname = 'project.dev' LIMIT 1

It takes 17ms in my local development box, and the EXPLAIN looks like:

1	SIMPLE	t0	ALL	IDX_16C8119E743BF776,IDX_16C8119E3101778E	NULL	NULL	NULL	1	Using where
1	SIMPLE	t17	eq_ref	PRIMARY	PRIMARY	4	dbname_dev.t0.default_locale_id	1	
1	SIMPLE	t23	eq_ref	PRIMARY	PRIMARY	4	dbname_dev.t0.base_currency_id	1	

Basically it is doing a table scan (even though small, probably nobody has that much queries).

Adding an index on the hostname column (maybe we should even make it unique, but that's another discussion and harder to upgrade) brings the query time back to 2ms for me and results in the following EXPLAIN:

1	SIMPLE	t0	ref	IDX_16C8119E743BF776,IDX_16C8119E3101778E,hostname	hostname	768	const	1	Using index condition
1	SIMPLE	t17	eq_ref	PRIMARY	PRIMARY	4	dbname_dev.t0.default_locale_id	1	
1	SIMPLE	t23	eq_ref	PRIMARY	PRIMARY	4	dbname_dev.t0.base_currency_id	1	
```

Table scan is gone and the index is solely used on this query. 

@stefandoorn stefandoorn changed the base branch from master to 1.1 January 2, 2018 18:15
@stefandoorn stefandoorn changed the title Add index on Channel::hostname to prevent table scan on each request (replaced #9080) Add index on Channel::hostname to prevent table scan on each request Jan 2, 2018
@pamil pamil merged commit 7099abf into Sylius:1.1 Jan 2, 2018
@pamil
Copy link
Contributor

pamil commented Jan 2, 2018

Thanks Stefan! 🥇

@@ -0,0 +1,28 @@
<?php declare(strict_types = 1);
Copy link
Member

Choose a reason for hiding this comment

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

something went wrong here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should it go away? Not sure it makes a lot of difference on a migration though, but it's good to be strict on it.

Copy link
Member

Choose a reason for hiding this comment

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

I just prefer to be coherent here. If you have a free moment I would love to merge a PR :)

/**
* Auto-generated Migration: Please modify to your needs!
*/
class Version20180102140039 extends AbstractMigration
Copy link
Member

Choose a reason for hiding this comment

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

missing final?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Other migrations I checked also didn't have final. Should I add it?

Copy link
Member

Choose a reason for hiding this comment

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

Just a suggestion :) I believe, that migration shouldn't be extended at all, but not sure about BC here ;)

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

Successfully merging this pull request may close these issues.

None yet

3 participants