Navigation Menu

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

Sql optimise new index merged branch #10284

Closed
wants to merge 45 commits into from
Closed

Sql optimise new index merged branch #10284

wants to merge 45 commits into from

Conversation

alikon
Copy link
Contributor

@alikon alikon commented May 7, 2016

Pull Request for fix conflict of #4115 .

Summary of Changes

conflict fixed and added mssql and postgresql compatibility

Testing Instructions:

This PR consists all the PRs merged in to this one regarding the index additions. Two approaches can be followed in order to test this PR. One is in an already installed site and one is for fresh distributions.

For already installed sites

For already installed sites first apply the PR and then add follow the bellow commands in order to add the indexes manually to the database

  • Index added to #_languages table (You can see this query execution when you access the site)

ALTER TABLE #__languages ADD INDEX 'idx_published_ordering' ('published','ordering'); (#11)

Related Query Before changes

  SELECT *
  FROM #__languages
  WHERE published=1
  ORDER BY ordering ASC

Related Query Before changes

  SELECT lang_id,lang_code,title,title_native,sef,image,description,
                 metakey,metadesc,sitename,published,access,ordering
  FROM #__languages
  WHERE published=1
  ORDER BY ordering ASC

  • Index added to #_session table (Query executes every time a user access a page)

ALTER TABLE #__session DROP PRIMARY KEY, ADD PRIMARY KEY(session_id(32)) ( #8 )
ALTER TABLE #__session DROP INDEX time, ADD INDEX time(time(10))

Index Description
This Index added in order to speed up the session update query allowing the sub string index usages


  • Index added to #_template_styles table (You can see this query execution when you access the site)

ALTER TABLE #__template_styles ADD INDEX idx_client_id('client_id') ( #13 )

Index Description
Without applying this index the below query cannot use any index to retrieve the rows from the database. After applying this index it clearly shows in the explanation section that the query uses the index to retrieve the rows

Related Query

    SELECT id, home, template, s.params 
    FROM #__template_styles as s 
    LEFT JOIN #__extensions as e 
    ON e.element=s.template 
    AND e.type='template' 
    AND e.client_id=s.client_id 
    WHERE s.client_id = 0 AND e.enabled = 1

  • Index added to #__contentitem_tag_map table (You can see this query execution when you access the site)

ALTER TABLE #__contentitem_tag_map ADD INDEX idx_alias_item_id ('type_alias','content_item_id') ( #14 )

Index Description
Without applying this index the below query cannot use any index to retrieve the rows from the database. After applying this index it clearly shows in the explanation section that the query uses the index to retrieve the rows. Also one another change did relevant to this index changes. Which is removing the t.* from the relevant query and adding the table fields instead. This change can be found (#14) and please refer to that PR also.

Related Query Before changes

    SELECT m.tag_id,t.*
    FROM #__contentitem_tag_map AS m
    INNER JOIN #__tags AS t ON m.tag_id = t.id
    WHERE m.type_alias = 'com_content.article' AND m.content_item_id = 5196 AND t.published = 1
    AND t.access IN (1,1,5)

Related Query After changes

    SELECT m.tag_id,'id',t.parent_id,t.lft,t.rgt,t.level,t.path,t.title,t.alias,t.note,t.description,t.published,
    t.checked_out,t.checked_out_time,t.access,t.params,t.metadesc,t.metakey,t.metadata,
    t.created_user_id,t.created_time,t.created_by_alias,t.modified_user_id,t.modified_time,
    t.images,t.urls,t.hits,t.language,t.version,t.publish_up,t.publish_down
    FROM #__contentitem_tag_map AS m
    INNER JOIN #__tags AS t ON m.tag_id = t.id
    WHERE m.type_alias = 'com_content.article' AND m.content_item_id = 5196 AND t.published = 1
    AND t.access IN (1,1,5)

  • Index added to #_extensions table (You can see this query execution when you access the site)

ALTER TABLE #__extensions ADD INDEX 'idx_type_ordering' ('type','ordering') ( #16 )

Index Description
Without applying this index the below query uses filesort to retrieve the rows from the database, which is an expensive retrieval operation. After applying this index it clearly shows in the explanation section that the query uses the index to retrieve the rows except the filesort.

Related Query

    SELECT folder AS type, element AS name, params
    FROM #_extensions
    WHERE enabled >= 1
    AND type ='plugin'
    AND state >= 0
    AND access IN (1,1,5)
    ORDER BY ordering

  • Index added to #_menu table (You can see this query execution when you access the site)

ALTER TABLE #__menu ADD INDEX 'idx_client_id_published_lft' ('client_id','published','lft') ( #17 )

Index Description
Without applying this index the below query uses filesort to retrieve the rows from the database, which is an expensive retrieval operation. After applying this index it clearly shows in the explanation section that the query uses the index to retrieve the rows except the filesort.

Related Query

    FROM #_menu AS m
    LEFT JOIN #_extensions AS e
    ON m.component_id = e.extension_id
    WHERE m.published = 1
    AND m.parent_id > 0
    AND m.client_id = 0
    ORDER BY m.lft

  • Index added to #__viewlevels table
    (query Executes when clicking on the add new category in the admin console)

Add New Category

ALTER TABLE #__viewlevels ADD INDEXidx_ordering_title(ordering,title)( #21 )

Index Description
Without applying this index the below query uses filesort to retrieve the rows from the database, which is an expensive retrieval operation. After applying this index it clearly shows in the explanation section that the query uses the index to retrieve the rows except the filesort. Also changed the JROOT/libraries/cms/html/access.php file's query which is shown below by removing the GROUP BY clause since the PRIMARY KEY includes in the selected fields. And this reduce the execution time

Related Query Before Change

    SELECT a.id AS value, a.title AS text
    FROM ltzvy_viewlevels AS a
    GROUP BY a.id, a.title, a.ordering
    ORDER BY a.ordering ASC,`title` ASC

Related Query After Change

    SELECT a.id AS value, a.title AS text
    FROM ltzvy_viewlevels AS a
    ORDER BY a.ordering ASC,`title` ASC

Testing Instructions

nadeeshaan and others added 30 commits January 25, 2015 11:46
fix conflicts 1) #__contentitem_tag_map
fix conflicts 2) #__extensions
fix conflicts 2) #__extensions
fix conflicts 1) #__contentitem_tag_map
fix conflicts 3)  #__languages
fix conflicts 4)  #__menu
fix conflicts 3)  #__languages
fix conflicts 4)  #__menu
fix conflicts 5)  #__session
fix conflicts 6) #__template_styles
@richard67
Copy link
Member

Do all the new indexes work with utf8mb4? I ask because it seems to be a bit older.

@alikon
Copy link
Contributor Author

alikon commented May 8, 2016

@richard67 should work with utf8mb4 too, yes it is very old comes from gsoc 2014

@waader
Copy link
Contributor

waader commented May 8, 2016

@alikon Do you have a suggestion how to test the update behaviour as "database fix" can´t be used anymore and there is no update package?

ALTER TABLE `#__session` DROP PRIMARY KEY, ADD PRIMARY KEY (session_id(32));
ALTER TABLE `#__session` DROP INDEX `time`, ADD INDEX `time` (time(10));
ALTER TABLE `#__template_styles` ADD INDEX `idx_client_id` (`client_id`);
ALTER TABLE `#__contentitem_tag_map` ADD INDEX `idx_alias_item_id` (`type_alias`,`content_item_id`);
Copy link
Member

Choose a reason for hiding this comment

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

Should be

ALTER TABLE #__contentitem_tag_map ADD INDEX idx_alias_item_id (type_alias(100),content_item_id);
in order to work with utf8mb4, because the InnoDB storage engine has a maximum index length of 767 bytesm which is max. 191 characters in utf8mb4, and for consistence with all other indexes we should use max. 100 characters.

Correct idx_alias_item_id for mysql utf8mb4 and add missing name quotes on other indexes with lengths limits for columns.

The idx_alias_item_id might not have lead to an SQL error before with utf8mb4 in case if not in strict session mode, but it would for sure make problems later.

The missing quotes I have added are cosmetics only and do not do any harm. I texted this script with the modification I proposed here in phpMyAdmin.
@richard67
Copy link
Member

@alikon I just have sent a PR to your repo with a correction for utf8mb4, see my line note above:
alikon#20.

@richard67
Copy link
Member

richard67 commented May 8, 2016

@waader What do you mean with

"database fix" can´t be used anymore

?

@waader
Copy link
Contributor

waader commented May 8, 2016

This is what I did:

  • installed current staging
  • copied the changed files of this patch over the existing ones
  • went to Extension > Manage > Database
  • found this message: Database schema version (3.6.0-2016-05-06) does not match CMS version (3.6.0-2016-05-07). ...
  • ... but no details were shown
  • pressed the fix button and
  • the message went away
  • when I controlled the outcome of this action I found that no changes to the schema were made

With all the changes in the installer I concluded that this procedure is not possible anymore.

@richard67
Copy link
Member

richard67 commented May 8, 2016

@waader @alikon

Well as I just checked in "libraries/cms/schema/changeitem/mysql.php", the database fix does not handle "DROP PRIMARY KEY" and "ADD PRIMARY KEY" statements, so these will not be detected by the databaase fix as structural change and so will be skipped.

Furthermore, the "DROP INDEX" and "ADD INDEX" statements in 1 row are also not detected as a structural change, because the database fix checks only for index names, not for details.

But the "ADD INDEX" for really new indexes should have worked.

So as it is now, the schema updates of this PR will only be applied when using Joomla! Update component for updating, but not when using the "copy or extract the files and use the database fix" method.

But regardless of all that, the correction for utf8mb4 I provided above has to be made.

@wilsonge Please confirm what I found.

@andrepereiradasilva
Copy link
Contributor

we already have some sql on update to 3.6.0 that for what i understand from your comments will not work ... https://github.com/joomla/joomla-cms/tree/staging/administrator/components/com_admin/sql/updates/mysql

so the Database -> Fix behaviour needs to be Fixed 😉

@richard67
Copy link
Member

Yes, is all known issues. Just wanted to explain here so people will not test in the wrong way.

But despite of this, my correction for utf8mb4 has to be applied, otherwise 1 of these new indexes will not work because max index size exceeded.

@andrepereiradasilva
Copy link
Contributor

yes richard i understand that

Correct idx_alias_item_id for mysql utf8mb4
@andrepereiradasilva
Copy link
Contributor

I have tested this item ✅ successfully on 95ac0fc

Tested by patching + database fix

This is what I did:

  • Use current staging with sample data
  • Applied patch with the patchtester
  • Went to Extension > Manage > Database
  • Found some schema errors, pressed fix
  • In phpmyadmin confirmed the new indexes were created
  • Read the description and notice the changes in the queries

Done a code review and all seems fine.

PS: While checking i notice some intensive queries (example: SHOW FULL COLUMNS FROM xxxxx_assets) and others with "filesort" or "NO INDEX KEY COULD BE USED" but they are already there before the PR.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10284.

@csthomas
Copy link
Contributor

csthomas commented Jan 11, 2017

I want to test that PR.

I have 2 requests/questions:

  1. [DELETED]

  2. In #__contentitem_tag_map there is no unique/primary key. This is bad because mysql create hidden additional primary key for that.
    I suggest to create a UNIQUE KEY or PRIMARY KEY as in example:

ALTER TABLE `#__contentitem_tag_map` ADD UNIQUE `idx_cid_type_alias_tag_id` (`type_alias`, `content_item_id`, `tag_id`);

@brianteeman
Copy link
Contributor

Can you look at resolving the conflicts so that this can be tested please or is it so out of date that it should be closed?

@brianteeman
Copy link
Contributor

It has been 10 months since the request for this pr to be updated without reply. I am going to close it as an abandoned issue. It can always be reopened if updated

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

10 participants