Related Tracker - Tracker!

@test

Test results: viewlevels

Before patch: 0.97 ms
After patch: 1.50 ms

• Failed

Please regroup in one file the db alterations to ease testing and as anyway it would be necessary:
Examples:
https://github.com/joomla/joomla-cms/tree/staging/administrator/components/com_admin/sql/updates/mysql

@810 Test is not necessarily failed: If your test is with default small dataset, query time can increase a bit with indexes with small sets, but with big sets dramatically decrease.

@nadeeshaan Great work!

It is easy to review the great work that you did from a database optimization perspective, as it is well presented here. So i'll give my database optimization perspective comments one by one:

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

agree.

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

agree

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

shouldn't that be ?:

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

and the related query be changed to:

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 = 0
    AND e.enabled = 1
    WHERE s.client_id = 0

?

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

agree.

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

agree

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

agree

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

agree.

(my above comments are from a MySQL/MariaDB perspective only, which is anyway 99+% of Joomla's audience).

Thank you very much for helping to verify the indexing @beat. I will
definitely look at the #13 and correct the issue.
:)

On Sat, Aug 16, 2014 at 2:08 AM, beat notifications@github.com wrote:

@810 https://github.com/810 Test is not necessarily failed: If your
test is with default small dataset, query time can increase a bit with
indexes with small sets, but with big sets dramatically decrease.

@nadeeshaan https://github.com/nadeeshaan Great work!

It is easy to review the great work that you did from a database
optimization perspective, as it is well presented here. So i'll give my
database optimization perspective comments one by one:

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

agree.

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

agree

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

shouldn't that be ?:

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

and the related query be changed to:

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 = 0
AND e.enabled = 1
WHERE s.client_id = 0

?

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

agree.

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

agree

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

agree

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

agree.

(my above comments are from a MySQL/MariaDB perspective only, which is
anyway 99+% of Joomla's audience).

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

Nadeeshaan Gunasinghe
Department of Computer Science and Engineering
University of Moratuwa
Sri Lanka

@test I'm finding it much harder to test this one as it isnt clear exactly where each of the indexes should be tested

@brianteeman In the above testing Instructions comment I have made include the details about the index and the related query as well as where to find the query. If you have any issue of figuring out an index to be test please mention and I will definitely update the instruction
:)

@brianteeman you can use the explain on "the query" before apply the patch and after applying the patch

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

before apply the pr

after apply the pr

you should notice the use of the new index

@infograf768 added update file for ALTER Queries.
Thanks.

@test the @beat tip

the query before

 SELECT id, home, template, s.params
 FROM old_template_styles as s LEFT JOIN old_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;

and the relative explain

the query after

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

and the relative explain

I'm really not a fan of this changing of a * to the entire set of fields from the table. How much of an effect does this have on performance vs just the key changes?

I'm just worried that readability is severely affected. Plus if any db changes ever happen to these tables it means more places where these things need changes and where bugs can be introduced

@wilsonge Good points! 👍

You're right: indexes have MUCH bigger performance gains than rewriting * to fields lists. And specially if it's full fields-list, I doubt about any performance gains, and if there are any, that could be automated in code and not made manually for maintainability in case of added fields.

In MySQL, sometimes/often * is slightly faster than the full list. E.g. COUNT(*) can be faster than e.g. COUNT(id). In Postgress, it could be different in older versions.

@wilsonge It's really a good point. In future if there will be any additional field or need any field somewhere have to change it several places. We can check performance difference with and without a.*.
What do you think about it @alikon ?

sorry is my mental sql syntax check 1.0 alpha
that hate select * and replace it with the field list

the right thing to do is to select the only "app" needed fields
despite in several db it may perform different this is not the "point"

i think is more easy to understand what that query "ask"
and even if it perform worst i'd prefer cause the code is more readable

Can you add the indexes for postresql and mssql too?

@nadeeshaan

Github say that
This pull request contains merge conflicts that must be resolved.

Can you have a look into it?

This PR is conflict is now solved and ready to review. Thanks everyone for review and tests.

the Query
SELECT m.core_content_id,m.content_item_id,m.type_alias,COUNT( tag_id) AS count,ct.router,cc.core_title,cc.core_alias,cc.core_catid,cc.core_language,cc.core_params

FROM sbgcn_contentitem_tag_map AS m

INNER JOIN sbgcn_tags AS t
ON m.tag_id = t.id

INNER JOIN sbgcn_ucm_content AS cc
ON m.core_content_id = cc.core_content_id

INNER JOIN sbgcn_content_types AS ct
ON m.type_alias = ct.type_alias

WHERE m.tag_id IN (2,3,4,5)
AND t.access IN (1,1)
AND (cc.core_access IN (1,1) OR cc.core_access = 0)
AND (m.content_item_id <> 24 OR m.type_alias <> 'com_content.article')
AND cc.core_state = 1
AND (cc.core_publish_up='0000-00-00 00:00:00' OR cc.core_publish_up<='2015-08-21 11:02:01')
AND (cc.core_publish_down='0000-00-00 00:00:00' OR cc.core_publish_down>='2015-08-21 11:02:01')

GROUP BY m.core_content_id,m.content_item_id,m.type_alias,ct.router,cc.core_title,cc.core_alias,cc.core_catid,cc.core_language,cc.core_params

ORDER BY count DESC
LIMIT 0, 5
Tested has a Query Time of 15.00 ms. and it is slower than some MSQL Queries.

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
tested ! and all the others MYSQL QUERIES.

I have tested this item 🔴 unsuccessfully on 7e0ed08

Confirmed for all but #17, see screenshot. I assumed that also here the filesort shouldn't be used anymore

@euismod2336 That's not necessarily an unsuccessful test! Specially if master has no better result... Many other improvements, worthwhile ones were made here. :-)

sure an adequate index seems to exist: (client_id, published, lft) , and would be most probably used by MySQL depending on the size of the result. The issue here for not being able to use straight that index is the m.parent > 0 in the where statement. You should try again with maybe 1000 or 10000 rows to see if MySQL uses that index.

No but this PR does conflict because it predates all the utf8mb4 index changes - there's no point in testing this unless the branch is updated again

For when @brianteeman hits this in a few weeks at the sprint tho - this is one that needs redoing - not just closing. It's going to give big improvements for "free"

@beat i agree that the improvements are considerable, but not all issues were "fixed" (as far as i could see), i'll have another go at it with more records if it might change it. all the other queries showed direct results.

@wilsonge not sure about the utf8mb4 changes, but this was PR was tested on the staging branch.

I have tested this item ✅ successfully on 7e0ed08

After adding 10.000 records the query is done properly on the index.

Thank you @beat for pointing this out.

Set to "closed" on behalf of @zero-24 by The JTracker Application at issues.joomla.org/joomla-cms/4115

Closing as we have a new updated PR at #10284 by @alikon