User tests: Successful: Unsuccessful:
Pull Request for Issue #12983
This is also an example of how GROUP BY in other queries should be revised
[EDITED]
Open backend menu "Menus",
Edit with e.g. phpmyadmin and set 2 home pages menu inside the same MENY Type (e.g. Australian Parks MENU), and set homes e.g. an English and a French
(previous behaviour makes the MENU appear twice with 2 language flags, like if there are 2 MENUs with same name)
Status | New | ⇒ | Pending |
Category | ⇒ | Modules Administration |
Labels |
Added:
?
|
AFAIK the group by clause is also need in sql mode ONLY_FULL_GROUP_BY.
Actually this will be enabled in 4.0 (see #12494 for more info)
@andrepereiradasilva
(yes i said that in my answer in the original issue)
but i see that i wrongly did not document this in the PHP comment
i have corrected my PHP comment about group-by to include this case too (thanks)
if i understand correctly the original issue #12983 was produced by a bad or misconfigured mysql instance of uniformserver.
with your proposed solution, you are shifthing the memory load from DB to PHP, plus issuing an additional query, i've not performed any perfomance test yet, but i'm not very convinced by this solution
2 faster queries that cost A time totally, instead of a single slower query that cost 2A or 5A or 10A
(in websites with many menu items)
is better right ?
(for small sites there is not performance gain only less memory needed by DB server for sorting)
Also the PHP loop that you mention, which adds the language data has almost cost 0,001 ms ? since it concerns MENUs and not MENU items,
so there is virtually zero PHP load added
effectively this change is win-win
1 more comment,
why join with language table to get 99% repeated language data (in websites with many menu items)
that are then used in both SELECT and GROUP BY clause wasting memory and CPU cycles ?
instead of doing a 1-table query that retrieves 3 or 5 language data rows, and doing a PHP loop that is 10 iterations over the menu types ?
maybe using the new ... JLanguageHelper::getContentLanguages()
?
thanks for taking time to look into my PR ! appreciated !
Please read below,
looking into this further, the existing Group By is not only somewhat lacking in terms of memory sort buffer and a little in terms of performance, but it is also has a bug, it fails to do its original purpose
Editthe menu items of a menu type (e.g. via phpmyadmin)
and set 2 language home pages in it e.g. English and French,
Please see the pictures below, existing code does this:
I have changed Group-By to be:
// Group by menu type id to count the (language) home page menu items inside it
// 0: no home, 1: home page, >1: misconfiguration detected
$query->group('a.id');
Thus you get both correct detection and even better memory and performance result
original purpose is to detect multiple (language) home pages inside same menu type (a misconfiguration allowed by older Joomla versions or e.g. by some bogus 3rd party code or other)
my next question is, what is the expected/correct behaviour ?
my next question is, what is the expected/correct behaviour ?
Propably existing behaviour is not what is desired,
-- because the group-by purpose is to count home page menu items inside the same menu
-- and because when a menu (Type) has 2 home page menu items, it will be show twice, (see) the picture, like if there are 2 menus with same name, but there is actually only one menu
-- also the layout code of the module expect a count, and furthermore if you do not want to count the home page menu items, then we could remove the group-by completely and still have the same effect because the joins are LEFT and we are grouping-by against all columns
Title |
|
Title |
|
I have tested this item
this will not work in full group by sql_mode (normally used by postgresql and sqlsrv and will be used in 4.0 for mysql too)
Make the changes made in this PR in mysqli driver and you will see the issue
https://github.com/joomla/joomla-cms/pull/12494/files#diff-21345ddf44224657f5e2f7a0ea63d7d5
i think something like this would work
$query->group('a.id, a.menutype, a.description, a.title, a.asset_id, b.language');
The fact that with:
$query->group('a.id, a.menutype, a.description, a.title, a.asset_id, b.language');
the query will not throw an error, and it will run in STRICT mode, and in other DBs,
The query is even more flowed that i originally thought
I will make a fix so that the query both runs in strict mode (and in other DBs) but also the resulting calculation is correct ! and no fake duplicates appear under the menu
- Fix the counting of multiple home page menu items inside the same MENU (Type) (see here: #12991 (comment))
How did you get this. every time I try I get the error message
Save failed with the following error: A menu should contain only one Default home.
How did you get this. every time I try I get the error message
yes, since some Joomla version N.N.N,
a check was added to prevent misconfiguration (i did not check the exact version that this was added)
so i manually edited the database table #__menu via phpmyadmin, and set 2 or more menu items (beloning of same menutype) to have home property (column) to 1,
(please set menu items of different language to see the effect of duplication better)
2 ways it can happen without editing
[EDITED]
if we do not care to have this check, then we can remove the SUM and the GROUP-BY completely, but it seems that this check of misconfiguration is desirable, so it exists
sorry i am not wasting my time looking at issues that cannot be created
without hacking the database
On 26 November 2016 at 13:45, Georgios Papadakis notifications@github.com
wrote:
How did you get this. every time I try I get the error message
yes, since some Joomla version a check was added to prevent
misconfigurationso i manually edited the database table #__menu via phpmyadmin, and set 2
or more menu items (beloning of same menutype) to have home property to
1,
(please set menu items of different language to see the effect of
duplication better)2 ways it can happen without editing
- some older Joomla version that did not prevent this, and user had
set it in the past- 3rd party code bogus code (or Joomla code) that will by erroneously
set 2 menu items to be home page—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#12991 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8ZDdrPchlZGFlPHlZuiK-DCx65Tuks5rCDfsgaJpZM4K66Sp
.
--
Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
https://brian.teeman.net/ http://brian.teeman.net/
sorry i am not wasting my time looking at issues that cannot be created without hacking the database
right, but this PR not only about the correct misconfiguration check
which is rare
If we do not care to have this check, then we can remove the SUM and the GROUP-BY completely, but it seems that this check of misconfiguration is desirable, so it exists
and it is better if it is fixed not be resource wasteful
Labels |
Removed:
?
|
Done fixed the query error,
query now
About adding the language data seperately after the Query,
there is no longer a reason to do it so i re-added the languages table JOIN,
(main query, now does not have WHERE clause and neither it has GROUP-BY clause)
Also i have installed postgresql and tested with it, also tested with MySQL with ONLY_FULL_GROUP_BY
the behaviour, which works ok here, is only one home menu item per language, including 'all', and only one home per menu
god job! will test it
seems to work fine.
what i'm failing to see in the memory usage and performance improvements in opposite to the group by solution.
can you be more specific how did you test that?
This works fine on mySQL here.
what i'm failing to see in the memory usage and performance improvements in opposite to the group by solution.
The Group-by did not disappear, we just revised it to a minimal sub-query and then a JOIN
For memory usage, i mean the sort buffer used by Group BY, it is obvious,
For performance i made some repeated tests, and DEBUG console showed 10% 20% faster Query (if my DB was not on SSD and with larger menus (if you have a few thousand of menu items) it should make performance more obvious another 10% or more)
To see bigger performance benefits from the smaller Group by (like this PR does)
you would need more rows to participate in the Grouping
There are other SQL queries that would benefit quite more by similar change
ok i understand, so how do i test performance/memory benefits?
1000 menu items?
ok i understand, so how do i test performance/memory benefits?
This PR is more
But if you want to see performance difference on this PR,
you need a lot of home page menu items or a lot of menu-types or both of them
e.g. Backup __menu_types (to restore it after testing) and then execute
INSERT INTO `PREFIX_menu_types` (asset_id, menutype, title, description) SELECT asset_id, FLOOR(RAND() * 99999), title, description FROM PREFIX_menu_types;
Execute it until you have e.g. 350 menu types, if it fails (reports a duplicate index) just rerun until you get 350 menu types
Then also add a description to them
UPDATE `PREFIX_menu_types` SET description=CONCAT('this is a test this is a test this is a test this is a test this is a test this is a test this is a test this is a test this is a test this is a test this is a test this is a test this is a test this is a test', FLOOR(RAND() * 99999))
For mysql or mariadb server you can do proof as below.
show status like 'sort%'
--------------
Variable_name Value
Sort_merge_passes 0
Sort_range 0
Sort_rows 0
Sort_scan 0
--------------
SELECT a.*, SUM(b.home) AS home, b.language, l.image, l.sef, l.title_native
FROM j25_menu_types AS a
LEFT JOIN j25_menu AS b ON b.menutype = a.menutype AND b.home != 0
LEFT JOIN j25_languages AS l ON l.lang_code = language
WHERE (b.client_id = 0 OR b.client_id IS NULL)
GROUP BY a.id, a.menutype, a.description, a.title, b.menutype,b.language,l.image,l.sef,l.title_native;
--------------
[RESULT, I had 20 rows]
--------------
show status like 'sort%'
--------------
Variable_name Value
Sort_merge_passes 0
Sort_range 0
Sort_rows 20
Sort_scan 1
show status like 'sort%'
--------------
Variable_name Value
Sort_merge_passes 0
Sort_range 0
Sort_rows 0
Sort_scan 0
--------------
SELECT a.*, c.home, c.language, l.image, l.sef, l.title_native
FROM j25_menu_types AS a
LEFT JOIN (
SELECT SUM(home) AS home, MIN(language) AS language, menutype
FROM j25_menu
WHERE home != 0 AND (client_id = 0 OR client_id IS NULL)
GROUP BY menutype) AS c ON c.menutype = a.menutype
LEFT JOIN j25_languages AS l ON l.lang_code = language;
--------------
[RESULT, I had 20 rows]
--------------
show status like 'sort%'
--------------
Variable_name Value
Sort_merge_passes 0
Sort_range 0
Sort_rows 1
Sort_scan 1
SET SESSION sort_buffer_size = 96 * 1024;
ERROR 1038 (HY001) at line 3: Out of sort memory, consider increasing server sort buffer size
I have tested this item
Code review + real test on joomla staging.
In order to see error add:
mysqli_query($this->connection, "SET SESSION sort_buffer_size = 64*1024;");
below line 187:
https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/database/driver/mysqli.php#L187
In order to see error add:
mysqli_query($this->connection, "SET SESSION sort_buffer_size = 64*1024;");
below line 187:
https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/database/driver/mysqli.php#L187
You mean of course how to see the error that this PR fixes
-- thanks for all the good feedback posted
Yes,
64*1024 is as example, it may depend on menu table size and I tested it on fresh install joomla.
For bigger menu table it may be a little more, but after that PR mysql/mariadb always requires less memory.
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2017-02-12 05:32:37 |
Closed_By | ⇒ | ggppdk |
Status | Closed | ⇒ | New |
Closed_Date | 2017-02-12 05:32:37 | ⇒ | |
Closed_By | ggppdk | ⇒ |
Status | New | ⇒ | Pending |
Besides performance and memory,
does it also handle the case of misconfigured sites to have 2+ home menu items for same language ?
No. I'm just simplifying query and remove GROUP BY statement.
I suggest for misconfigured menu item to fix it in other way.
We should not hide any errors, because it may generate other weird behaviour on front end.
May be it can be fixed by click on home button to remove home on 2+ menu items and then set up home again.
Or create some update sql file which removes illegal home=1 from menus which has more than 1 home.
I suggest for misconfigured menu item to fix it in other way.
No, i am not adding any new fix here,
i am only patching the DB query so that existing behavour works (backend templating for displaying the home menu items)
Just apply the fix and add 2 home page menu items by editing menu items DB table for same language,
and you will see that what existing backend template for displaying home menu items will do
We should not hide any errors, because it may generate other weird behaviour on front end.
A warning message would be enough,
user will edit add remove the home page menu item that is not needed,
not worth to add any more effort to this, as it is rare,
the code mainly catches cases of old Joomla versions that allowed setting 2 home page MENU items inside same MENU, or rare case that some software (e.g. 3rd party) would create 2 such menu items
After a discussion in bugsquad, I suggest to let open a future implementation of multiple home pages (when set to a lang else than ALL). This would be done in the libraries.
I.e. I suggest to just concentrate on the faster query if really necessary.
1.7 is a dead fish.
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2017-03-08 18:07:06 |
Closed_By | ⇒ | ggppdk |
@ggppdk AFAIK the group by clause is also need in sql mode
ONLY_FULL_GROUP_BY
.Actually this will be enabled in 4.0 (see #12494 for more info)