Doesn't this need to be in the update SQL as well?
On 6 Jun 2016 10:57 a.m., "Nicola Galgano" notifications@github.com wrote:

Follow up #10710 #10710 .
Summary of Changes

added an index on field alias for content table to speed up alias
duplicate check
Testing Instructions

see #10710 #10710

You can view, comment on, or merge this pull request online at:

#10739
Commit Summary

• [com_content] - use index for check duplicate alias

File Changes

• M installation/sql/mysql/joomla.sql https://github.com/joomla/joomla-cms/pull/10739/files#diff-0 (3)

Patch Links:

• https://github.com/joomla/joomla-cms/pull/10739.patch
• https://github.com/joomla/joomla-cms/pull/10739.diff

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#10739, or mute the thread
https://github.com/notifications/unsubscribe/ABPH8b_wQ5BgBvYLwfpQV95UTIUJF8p4ks5qI-7rgaJpZM4IuvI5
.

yes there are

I must stop reading pr on my phone

This will not work with utf8mb4. The index has to be limited to the first 100 characters of the alias column for mysql in the update script and in joomla.sql as we did for the other indexes, too, where the column is so long that it would exceed the maximum index size (in bytes).

@richard67 what about innodb-large-prefix=true ?

@alikon I have no experience with that. From reading PHP manuals I am not sure if this option can be always set (there might be restrictions on that), but if it can, it would make it possible to use the full columns for the indexes and so of course be the best solutuion.

But in this case we should also change other indexes where we currently use the 1st 100 chars only back to using the full columns, e.g. the many "idx_alias" indexes we have, so everything is consistent.

I am not really a mysql expert, so maybe you should discuss that with someone else, or maybe ask PLT to decide, e.g. @wilsonge (he could ask Eli, I do not find Eli here with the "@" method).

I could help to identify what all has to be changed if there was a decision to go that way. Otherwise, if PLT is against it or it will not always work with the innodb-large-prefix option, you can apply to your PR here the same change which you once got from me with your other PR before, with limiting the column in the index to the 1st 100 chars.

You know I was helping with the utf8mb4 conversion stuff as far as I could, but I was also new to all that stuff, and so meanwhile after having learned a bit more I see we could have done many things better, and some stuff (e.g. enlarging varchar columns from 250 or 255 to 400) was not really necessary.

And that keeps me a bit from continuing work on it, e.g. on utf8mb4 conversion for extensions, because there would be much to clean up, and I do not want to repeat for the extensions the mistakes we (me but not only me) made with utf8mb4 conversion for the core.

@aschkenasy your input would be appreciated on this

I have tested this item ? unsuccessfully on 06c3f43

Followed Test instructions in #10710 but could only find SELECT * FROM __content WHEREid= '1'. Is this Patch to test or do i miss something?

@alikon Any updates here?

well the simplest thing we could do is to go with:
ALTER TABLE '#__content' ADD INDEX 'idx_alias' ('alias'(191));
for mysql only as it is the only one affected by utf8mb4 key limit

cause go for

• innodb_large_prefix = 1
• innodb_file_format = Barracuda
• ROW_FORMAT=DYNAMIC

should be a little bit more complex

but i'd want to listen @aschkenasy

@alikon I agree with your previous statement. We should not forget to add this to the utf8mb4 conversion scripts, as it is with other indexes. If necessary I can help with this.

@franz-wohlkoenig Where are the "details following" you promised with your test result?

@richard67 i wrote "Followed Test instructions …" … or you mean something different?

@franz-wohlkoenig It seems the issue tracker is not up to date with the text on GitHub, see screenshot from issue tracker:

Sorry for confusing you.

@richard67 got it. To be honest: I can't remember.

@alikon You should rename your schema update sql scripts to 3.7.3-2017-06-03.sql or later date. Otherwise they will not be executed on a Joomla version between 3.6.1 and 3.7.2 when updating to 3.7.3 which will include this PR if all good.

can you help me with the coversion script?

@alikon You mean me? Conversion scripts look ok, but installation/sql/sqlazure/joomla.sql has big conflicts.

i'm trying to solve the conflict .... so no need to do things on the conversion script side @richard67 ?

@alikon Ah you mean those for utf8mb4 comversion .. those have to be updated, too.

@alikon Done, see PRs 28 and 29 against your repository.

@alikon Theoretically, the changes in the utf8mb4 conversions scripts are not necessary, because your index was right from the beginning on and so has not been modified. But I remember that when I had made the utf8mb4 stuff, we had errors with mysql complaining about lengths of indexes during converting the charset of the table even when the index length limit was already ok, so it is not a bad idea to add that to be on the safer side.

thank you @richard67

@alikon Sorry, my mistake. Could you just change "ADD INDEX" to "ADD KEY" in the 2nd conversion script? Just to be consistent, it would work also with "ADD INDEX" but does not look nice if all the others are "ADD KEY".

@aschkenasy could be a good idea for joomla 4.0

@alikon Unfortunately, if my PR #16469 will be merged (it has already RTC), you might have merge conflicts again because I also used 3.7.3-2017-06-03.sql as name of some new schema updates. You can avoid this by renaming your schema updates to a different name, e.g. 3.7.3-2017-06-07.sql before my PR is merged.
I recommend to use separate files for my and your PR also because we should not put too many things which are not related to each other in 1 schema update file.

changed file date name as requested for avoid conflict with #16469

I have tested this item ✅ successfully on 0f9d5fd

I tested as described here: #10567 (comment), including the modification of the file "/libraries/legacy/controller/form.php" desrcibed there.

First I created an article without this PR here being applied, then I applied this PR using the patchtester and went to "Extensions -> Manage -> Database" and used the "Fix" button to apply the schema update from this PR, means creating the index, then I again created an article in the same category as before, all having the "debug system" switched on in global config.

Without this PR applied, the query took longer than after, and after it used combined index on catid and alias while before it uses only catid.

See the screenshots I will provide with a comment soon.

Before applying this PR:

After applying this PR:

@franz-wohlkoenig Can you test this PR here again? See comment in my test result on how I tested. See screenshots of my result below my test result. This PR here is old but still good and very much worth to be merged.

@richard67

1. file "/libraries/legacy/controller/form.php" modified,
2. Debug enabled,
3. no PR applied,
4. new Article saved.

Got:

Which Query i have to look on?

@franz-wohlkoenig as shown in my screenshot, the first one which selects from the content table where alias = x and catid = y. Should be the first statement at all selecting somthing from the #__content table.

found nothing like in your Screenshot. Similar is:

but no catid.

@franz-wohlkoenig You have to select a category for the article before using the save button. The category should not be "uncategorized".

@franz-wohlkoenig And if no catid, what do you see when you click the "explain" link to expand the explain plan of that query? does it show if an index on the alias is used? before patch it should not show that, after patch it should show that.
Maybe you have only 1 category?

A Category is select, its not "uncategorized", more than 20 Categories (sample data).

Strange, I have no idea why your queries are so different to mine.

@franz-wohlkoenig You create a new article, select an existing category, then use the "save", not the "save and close" button, then check all queries. Somewhere really should be a query like I have mentioned.

@franz-wohlkoenig P.S. you can do that on the same page, no need to open a new tab as that comment in that other issue was telling.

new Article in existing Category saved (not "save and close"), Queries checked in same Tab = same Result as above:

@franz-wohlkoenig to see the changed query with the debug plugin when saving an article in backend with user =admin you need to do a simple dirty hack
Edit file: https://github.com/joomla/joomla-cms/blob/staging/libraries/legacy/controller/form.php#L831

and add before return

if ( $context == 'com_content.edit.article' && JFactory::getUser()->username=='admin' && $app->isAdmin() )
{
    $this->setRedirect(null);
}

@franz-wohlkoenig Maybe it depends on how you create the new article which queries are run? I did it in that way:

And then in the article edit view, I filled in a title, some text, chose an existing category which already contains some extensions, then clicked save button and then checked the query in the debug info. Of course I have applied the hack which @alikon mentioned above before I did that all. And I cleared all Joomla cache after applying the hack before the test without PR, and then again before applying the PR for the 2nd test.

@ggppdk I ask you because you were involved in #10710 : Do you think you find time and mood to test this PR here? I am sure it will be an easy task for you.

@franz-wohlkoenig P.S. In the hack of the PHP file your might have to replace "admin" by your superadmin's user name.

Using Super Admins username give after Save only Debug-Console back, no Article (Aricle is saved, but locked). So Result is same, no cat-id is shown:

@franz-wohlkoenig Are you sure you are testing on Joomla and not WP? ... Joking.
No, I have no idea what could be the difference causing the difference in our queries.

hoping for a second Tester …

I had the hope that @ggppdk could find time to help with a test because he was involved in #10710 .

today was next Report on this Issue.

@franz-wohlkoenig Where do you mean?

#16732

@franz-wohlkoenig maybe you mixed up PRs? I think you wanted to write your comment to #16577 and not this one here.

I have tested this item ✅ successfully on 0f9d5fd

Before this PR:

After this PR:

Attention: For other testers this PR can not be full revert with the Revert-Patch-Button in the Patch-Tester because the Database is changed !

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/10739.}

RTC after two successful tests.

@franz-wohlkoenig and all maintaners and @alikon: Not commit this yet. It needs to change the names of the update files so they fit to actual version situation.
Change 3.7.3-2017-06-08.sql to e.g. 3.8.0-2017-09-10.sql if it will go into 3.8.0, and to something later if it will not go into 3.8.0.
After name change it will not need functional tests again from my point of view; a code review should be ok after that final change.

@alikon Can you rename the update sql files?

Status back on "Pending" as stated above.

@alikon To make it more comfortable for you to rename the SQL schema update files: See alikon#35.

thanks @richard67 merged

I have tested this item ✅ successfully on 60751ad

Last commits only changed names of the schema update SQL scripts so they fit to the next Joomla version, so my previous test is still valid.

@Sieger66 Can you mark your test result as successful again? No need to test again. Last commits only changed names of the schema update SQL scripts so they fit to the next Joomla version.

I have tested this item ✅ successfully on 60751ad

@franz-wohlkoenig RTC?

RTC after two successful tests.

@richard67 Thanks for Hint.

Now as Joomla 3.8.0 is released, it needs again to rename the schema update SQL scripts e.g. to 3.8.1-2017-09-19.sql.

@alikon Shall I again prepare a PR against your repo for your convenience?

@franz-wohlkoenig Should the RTC be removed until the schema update SQL scripts have been renamed? I don't know and leave it up to you.

@richard67 Maintainers should decide, so i let it as is.

@franz-wohlkoenig OK.

@franz-wohlkoenig Any news on this?

@mbabker?

I missed the RTC on this, sorry.

Just rename the SQL files for 3.8.2 (preferably with today's date too) and I'll merge it.

@alikon See alikon#37 for rename of the schema update SQL scripts.

@mbabker Done. Merge please.

@richard67 thanks