@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)

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() ?

wait, wait, i'm not saying your solution is wrong....

i've not performed any perfomance test yet

and maybe i like more sql than php ?

for example #12999

@alikon

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

• and its 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)

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 ?

@infograf768

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

I have tested this item ? unsuccessfully on 900c156

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,

• does not mean that the calculation is correct by addng the above group-by, the query just runs without error and produces bad data, since it will

1. Fail to count duplicates home page menu items if they are of different language
2. Will create a duplicate display of the menu type under the "Menus" as if 2 menu type exists with the same name

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

1. 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

• 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 erroneously set 2 menu items to be home page

[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
misconfiguration

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 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

• this PR is mainly about a resource wasteful query, (especially when have many menu items)

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

Done fixed the query error,
query now

• uses much less memory (due to both minimized select and single column group-by via a subquery)
• 10% - 20% faster, maybe more benefit with larger menu table, but i saw that indexes helped old query too (so old query did not seem to have much of a performance issue)
• yields correct results with no duplicates (see picture in my previous comment with the Globe icon)

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,

• just look at the length of the GROUP BY ! 90% less memory needed by the sort buffer

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

• a larger table with a WHERE clause that does not exclude 98% of the rows (which is happening in this Query)

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

• about sort-buffer memory and correct detection of duplicates (single MENU appears with Globe)
• and about providing an example on how to deal with unneeded large Group By in other queries that group-by on a lot of rows

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.

1. You can login to mysql command line (mysql -ujoomla_user -p database_name) or write your own script to see similar results. Mysql has some statistics about used buffers per connection.

QUERY BEFORE PATCH

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

2. As you can see sort rows is used 20 times ([updated] 20 rows was sorted).
3. Close connection and reconnect.

QUERY AFTER PATCH

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

4. Now sort rows is used only once!
   [updated]
5. You can decrease sort_buffer_size (for tests) by:
   SET SESSION sort_buffer_size = 96 * 1024;
6. Then for me not patched version does not work and return error:
   ERROR 1038 (HY001) at line 3: Out of sort memory, consider increasing server sort buffer size

I have tested this item ✅ successfully on c7135c2

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.

I have created a different implementation at #14011
Can you take a look and test?

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.