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

Remove outdated MyISAM support. Engines are smart enough to determine th... #2

Closed
wants to merge 2 commits into from

Conversation

Pandos
Copy link
Contributor

@Pandos Pandos commented Jan 30, 2014

...eir engine. MySQL uses InnoDB since 5.5 as default.

So for that in InnoDB, auto_increment keys have to have to: either have their own index, or at least be the primary sub-index of a compound index. (a in KEY(a,b)).
If not it throws ERROR 1075 (Incorrect table definition).

Signed-off-by: Sven Rissmann creaworld-media@gmx.de

… their engine. MySQL uses InnoDB since 5.5 as default.

So for that in InnoDB, auto_increment keys have to have to: either have their own index, or at least be the primary sub-index of a compound index. (a in KEY(a,b)).
If not it throws ERROR 1075 (Incorrect table definition).

Signed-off-by: Sven Rissmann <creaworld-media@gmx.de>
@Xarcell
Copy link

Xarcell commented Jan 30, 2014

Doesn't InnoDB have problems still? I can't remember the details, but certain field types in innodb can crash the whole mysql server.

@Pandos
Copy link
Contributor Author

Pandos commented Jan 30, 2014

I don't think so. Never have heard about it. Running a big forum with over 2 GB database on InnoDB without a problem for years.
Just a note:
If a read is slow or hasn't completed and a read/write is waiting on the first read to finish then the MyISAM table referenced in the read is held in a locked state till the resultset is made available to the query. This also causes a rise in the load average on the server and slows your site down. During this time no reads or writes can complete ofcourse as MyISAM only has table-level locking.
So the queries that are victims of lock escalations under heavy but slow reads would do much better as a table converted to Innodb.

…e used instead.

Signed-off-by: Sven Rissmann <creaworld-media@gmx.de>
@eurich
Copy link
Contributor

eurich commented Jan 30, 2014

IIRC older MySQL Versions don't support the full text search index with InnoDB.

@Nao
Copy link
Member

Nao commented Jan 30, 2014

  • @Pandos, never heard of these auto_increment problems. I'll look into them.
  • Why did you invert auto_increment and not null in one query?
  • Please don't modify ENGINE=MyISAM stuff. If I were to apply your commit, it would completely break Wedge because install.php relies on that particular graft UTF8 support into tables. As a general issue: whenever you remove something, make sure it doesn't break anything else.
  • InnoDB is better than MyISAM in many areas, and worse in some (or many, depending on who you're talking you.) Changing from MyISAM to InnoDB is not trivial, and as I'm not a MySQL specialist, I'm not doing it without a good reason.
  • You can always switch your tables manually. I believe you did..?
  • What I'd be willing to do, though, is offer users the ability to choose their table format at install time. It's just a matter of a few lines in install.php, so it's very fine.
  • Please do not put two unrelated 'fixes' into the same pull request. This makes it harder to find what you changed. This one's acceptable, but as a general rule of thumb, it shouldn't be done.
  • Are you sure that Length function does the same as Size? It's just a name change? Nothing else? Is it supported in ImageMagick 1? It seems to me like it's IM2-only... I don't remember what version I have, and Dragooon added IM support in the first place (back when I didn't have it on my server), so I didn't check this kind of thing.
  • @eurich, I think full text search was removed from Wedge a few months ago. Pete was (and still is) obsessed with SQL search, as he participated in the Sphinx community before he joined SMF, and he always disliked one of the search options. I think it was the full text search index. Unless I'm mistaken. I'd need to go through the history. And honestly, I don't know if it's a good thing he did, because personally I never use search options much, and I'd rather implement Google Custom Search like I did on Noisen.com, if you were to ask.

@Xarcell
Copy link

Xarcell commented Jan 30, 2014

@eurich IIRC older MySQL Versions don't support the full text search index with InnoDB.

Yeah, that's it. I offer web hosting, and 99.9% of the time when the MYSQL database crashes, it'e because of this. Innodb is better overall, but has some instability. It may be better by now, I haven't had to sic my tech team on a crashed MYSQL in over a year.

@Pandos
Copy link
Contributor Author

Pandos commented Jan 30, 2014

A little bit malformed my description...
My commit doesn't remove MyISAM from any table.
For databases before 5.5 MyISAM is still used, because it's the default. This only applies for databases from 5.5 where InnoDB ist default. So there s no worry about it. It would'nt break anything!

Also I don't understand your concern about UTF-8. This change nothing to it. So it's save.
As far as I know Wedge always sending meta information for UTF-8.
To be save I could add a statement: SET NAMES 'utf8'.

FYI:
http://bugs.mysql.com/bug.php?id=45987

Auto_increment was changed because of my tries to add the index seperately. That doesn't make any diiference. If you wish I can twist them back.

So I only have created 1 pull request.
I've committed my changes to Imagick to my repo and it's automatic purged in that PR. :(
But nevertheless GH is better than BB :)

Imagick::getImageSize()Returns the image length in bytes. Deprecated in favour of Imagick::getImageLength().
Sorry, but anyone who is running the required PHP-Version will use PECL imagick 2.0.0. But most will use GD. But I'm not most ;)

@Xarcell
I'm running hughe db's on InnoDB for years.
It's more useful and healthy than MyISAM due to automatic repair of corrupted tables, etc.
It's really stable.
Just take a look at percona. :)

Something left out?

@Xarcell
Copy link

Xarcell commented Jan 30, 2014

@Pandos good for you. I was merely stating that I have experience quite a few MYSQL database crashes due to corrupted InnoDB tables. In some cases, the site could not even be recovered. I don't claim to know all the technical aspects, I am merely stating from experience and from what my tech teams tell me what caused the issue.

@Pandos
Copy link
Contributor Author

Pandos commented Jan 30, 2014

Hmm... I've experienced more unrecovered crashes from MyISAM instead of InnoDB.
InnoDB does auto recovery from crash via replay of logs.
Don't know what db version ran into this problems your tech team is awared of.

@Nao
Copy link
Member

Nao commented Feb 3, 2014

I'm still waiting for feedback on (most of) my post above... This PR will stay unchanged if nothing happens, because I don't even know what I'm expected to do.

@Pandos
Copy link
Contributor Author

Pandos commented Feb 3, 2014

Eh... it's already answered in my post?
OK, one thing left:
If you want to let the user choose their db layout, it's OK for me.
My commit is a handy solution for standard layouts.

@Nao
Copy link
Member

Nao commented Feb 3, 2014

You didn't get what I'm saying about ENGINE=...
Let's be clear: do a text search in the Wedge folders, and you'll find several occurrences of ENGINE=MyISAM, which is used as a 'hook point' for Wedge (and SMF) to modify other things.
Which means that your commit would effectively completely break this code, and make it impossible to update the tables.

@Pandos
Copy link
Contributor Author

Pandos commented Feb 3, 2014

Now I understand what you tried to say to me :)
OK, Cancel my PR. I'll have to rebase my repo anyway.
But the changes to Imagick are right! So I pull them again :)

For InnoDB support. Will look into it and we can discuss what will be the right way for it.

@Nao
Copy link
Member

Nao commented Feb 3, 2014

I'll take your imagick changes of course. ;)

@Pandos
Copy link
Contributor Author

Pandos commented Feb 3, 2014

Cancel complete PR please.
Will start new.

@Nao
Copy link
Member

Nao commented Feb 4, 2014

Or I'll cherry pick! I'll see.

@Nao
Copy link
Member

Nao commented Feb 4, 2014

Done. Oddly, though, it still offers me to Merge.. But, well, I won't!

@Nao Nao closed this Feb 4, 2014
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.

4 participants