Remove MyISAM Requirement #2442

ahosker · 2020-11-06T19:59:11Z

Fixes
The requirement for Database to be MyISAM Compatible.

DigitalOcean Managed Databases do not have MyISAM enabled and there is no way of allowing it. The solution offered by the forum was to not elegant.

If this is not have breaking changes, it is an improvement to the installation process for those with Limited Database Engines.

Changes proposed in this pull request:
Installation Changes
Edits Create Post, Migration table to remove hardcoded MyISAM requirement.
Edits Installation DatabaseConfig.php to remove hardcoded MyISAM requirement, leaving it up to the database to decide.

Reviewers should focus on:

Can you just edit old Migration Files? It shouldn't affect existing installations and will allow easier to install new installations. That do not have MyISAM.
What effect does setting engine to NULL have in real terms. Was MyISAM selected by default for a reason? Would it be better to fallback if MyISAM is not available?

Issue #1661 and #677 and #1675 are loosely relevant

Screenshot

tankerkiller125

I am fine with making InnoDB the default, overall InnoDB has better performance anyways, I'm not sure if theirs a valid reason it defaults to MyISAM.

askvortsov1

@tankerkiller125 Just history, we generally don't change old migration (although this seems to be a good reason to do so).

Aside from the StyleCI complaint, a few minor questions below

askvortsov1 · 2020-11-07T02:59:42Z

migrations/2015_02_24_000000_create_posts_table.php

@@ -31,7 +31,7 @@

            $table->unique(['discussion_id', 'number']);

-            $table->engine = 'MyISAM';


Is no default needed here? Why not explicitly default to Innodb

The issue I had was the Database did not support MyISAM. Wouldn't changing it to Innodb just bake in the same issue for a future version of me, with a Database that does not support Innodb. As such I determined that NULL (or no default) was the best option. Leaving it up to the Laravel Backend to determine.

askvortsov1 · 2020-11-07T03:00:02Z

src/Install/DatabaseConfig.php

@@ -47,7 +47,7 @@ public function toArray()
            'collation' => 'utf8mb4_unicode_ci',
            'prefix'    => $this->prefix,
            'strict'    => false,
-            'engine'    => 'InnoDB',
+            'engine'    => NULL,


Again, why mark this as null and not tell it to use innodb?

The issue I had was the Database did not support MyISAM. Wouldn't changing it to Innodb just bake in the same issue for a future version of me, with a Database that does not support Innodb. As such I determined that NULL (or no default) was the best option. Leaving it up to the Laravel Backend to determine.

tankerkiller125

After further review (on an actual desktop this time) I think I'd actually rather see a migration or migrations to update existing Flarum installs to InnoDB if we do this at all.

As far as actually doing this though I'm not 100% sure now after further research MyISAM is far more performant when it comes to a lot of read operations. However InnoDB does much better with a mix. Given that Flarum is forum software I'm inclined to believe that InnoDB would be better for it, but I don't have data on SQL reads vs writes on a active Flarum install.

luceos · 2020-11-09T08:44:51Z

For historical reasons, I'm going to pitch in.

The MyISAM requirement on the posts table was added to support Fulltext search. Which hasn't been supported by InnoDB for a very long time. We recently (ahem) updated our requirements to MySQL 5.6+ and Mariadb 10.0.5+, both of these versions now allow using fulltext indices to be created with InnoDB. So we can safely start doing this.

Two considerations:

We had in mind to separate the migrations into two categories: installation migrations and updates migrations. The benefits of that is that we can create new forums faster and prevent an issue where running all migrations on new installs using xhr can kill the process - on short timeout servers - and cause partially installed databases and a ton of fallout.
Running a migration to update to InnoDB on a multi million posts table is going to wreak havoc.

stale · 2021-02-09T17:24:02Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this to keep the amount of open issues to a manageable minimum.
In any case, thanks for taking an interest in this software and contributing by opening the issue in the first place!

tankerkiller125 · 2021-02-09T22:02:47Z

The last point from @luceos is a good point that running a migration against millions of post will be very bad. My proposal then would be to change all the current migrations to InnoDB (thus all new installs get that benefit) and then post a set of instructions to migrate existing databases manually (with a backup/restore of tables as part of that instruction set)

This ensures that new installs get the benefits, and if people want to take risk they can get them too. If they don't want to take risk though they can just simply leave things as is.

askvortsov1 · 2021-02-09T22:40:51Z

Not stale, but I propose we close this in favor of a discussion to work out exactly how we want to approach this.

ahosker · 2021-02-10T11:22:12Z

This commit does resolve an install breaking issue. That could have been resolved 3 months ago (how time flys), leading to (presumably) less install issues and greater adoption.

Committing migrations/2015_02_24_000000_create_posts_table.php is only removing the default on install. It does not as @tankerkiller125 outlines cause existing databases to be migrated to InnoDB or otherwise. Nor does it prevent a migration file at a later date to do that, if the community chooses.

I'd be repeating myself for Committing src/Install/DatabaseConfig.php this is (as far as I know) only used in install. It removes an install braking issue. It does not prevent future modifications as discussions continue.

I can view this as a Critical Commit but not as one that requires a prolonged discussion @askvortsov1 though there is, of course, discussions to be had related to this. Should MySSIAM be the default (instead of none)? Should old tables be migrated?

Thanks for the historical perspective @luceos a lot of thought went into these two lines in the past.

TL'DR. The guy who created the pull request obviously wants it committed. It fixed installs and can leave wider discussions on database engines to those who want to have them.

ahosker

Replacing Old Default with a New Default could create a future issue. Having no default allows the system to use the available Database Engine. This is a preference thing, and not firmly committed to no default if others believe a default database engine is preferable.

Other systems give the user the preference? Though would require further modificaitons.

luceos · 2021-02-10T11:57:17Z

I'm not really one to look into our open PR's that much, apologies for that. Here's what I think if we consider adopting this right away.

I see no changes that modify the migration that changes the posts table to MyISAM, so that migration will still be executed without need. This migration can be dropped. Existing installations won't complain and to new ones it doesn't matter. Alternatively modify that migration to check the engine in use and only run it when it's not MyISAM.
Do we need a test that confirms the engine selected (or rather defaulted) is the one that is being used?

As to not merging this in the past couple of months; we are pushing all effort in reaching stable. These PR's seem minor, but they cannot and should not be merged without proper evaluation. Open source isn't just about accepting code by others, but also about having to deal with every single repercussion way past author involvement.

tankerkiller125 · 2021-02-10T13:17:16Z

This commit does resolve an install breaking issue. That could have been resolved 3 months ago (how time flys), leading to (presumably) less install issues and greater adoption.

If your referring to fixing issues on DigitialOcean managed MySQL installs they have actually changed even more things in the past 3 months that permanently break installing Flarum that are well outside the scope of this projects goals.

askvortsov1 · 2021-03-01T22:40:49Z

TL'DR. The guy who created the pull request obviously wants it committed. It fixed installs and can leave wider discussions on database engines to those who want to have them.

I apologize for my stark wording in the comment above; often PRs are opened, and then activity on them dies. In those cases, discussion can get lost (closed issues are a better record of decision making closed PRs). I'm also sorry for our inactivity on this: we're in the middle of a large release, and our priority was stabilizing the extension API.

As Daniel noted above, we initially defaulted to MYISAM because InnoDB didn't support fulltext search. Now that it does, I am in favor of dropping the migration and changing the default. That being said, my main concern is that some databases could default to an engine that isn't InnoDB or MyISAM, and someone would end up with a partially broken Flarum install, as opposed to not being able to install due to requirements issues. That being said, I am not that familiar with how Laravel handles database engines (or the engines themselves for that matter), so I might be misinterpreting this.

luceos · 2021-03-01T23:07:35Z

If my recommendation to drop the initial engine change is applied, I would love to see a core dev do a test run with:

a clean install
an existing database

Then I'd be okay with merging.

askvortsov1 · 2021-03-01T23:53:45Z

an existing database

What'd be the benefit of that, since the migration is dropped?

luceos · 2021-03-06T11:22:48Z

What'd be the benefit of that, since the migration is dropped?

Yeah was hoping to confirm nothing breaks, but indeed the entry still exists in the database even though the migration files does not. So .. only a new install test would be required.

tankerkiller125

I've tested a fresh install using this PR. And while everything works I have concerns over the lack of a proper default engine setting.

My main issue around it is one that's already been stated: If no default is set and then someone sets a very strange engine as default in MySQL it may cause install errors for Flarum. It would be by recommendation that we revert the change in DatabaseConfig.php and only change the migration.

As for the concern over a database not having InnoDB, given our current MySQL/MariaDB requirements this is incredibly unlikely unless someone built MySQL without it on-purpose. In which case that user probably has the technical ability required to resolve the issue (not to mention the issues they would get with other PHP projects by not having InnoDB)

luceos · 2021-04-06T14:28:41Z

According to vendor/doctrine/dbal/lib/Doctrine/DBAL/Platforms/MySqlPlatform.php:

        // Engine
        if (! isset($options['engine'])) {
            $options['engine'] = 'InnoDB';
        }

Flarum will always fallback on InnoDB, unless database.options.engine is set in config.php.

tankerkiller125 · 2021-04-06T19:55:51Z

That works for me then.

askvortsov1

Before merging, let's replace the NULL with null, as that's more consistent with what we use.

src/Install/DatabaseConfig.php

askvortsov1 · 2021-04-08T03:25:24Z

src/Install/DatabaseConfig.php

@@ -47,7 +47,7 @@ public function toArray()
            'collation' => 'utf8mb4_unicode_ci',
            'prefix'    => $this->prefix,
            'strict'    => false,
-            'engine'    => 'InnoDB',
+            'engine'    => null,


Should we even specify this key if we're just setting it to null?

That's the convention in Laravel, yes. And if I'm not mistaken it exists also to indicate the options available in general.

askvortsov1 · 2021-04-08T03:26:21Z

src/Install/DatabaseConfig.php

@@ -47,7 +47,7 @@ public function toArray()
            'collation' => 'utf8mb4_unicode_ci',
            'prefix'    => $this->prefix,
            'strict'    => false,
-            'engine'    => 'InnoDB',
+            'engine'    => null,


Suggested change

'engine' => null,

askvortsov1 · 2021-04-09T12:14:31Z

Thank you again for the PR, sorry for the long review process on this one.

ahosker added 2 commits November 6, 2020 19:44

Remove Database Engine Default of InnoDB

30c0fed

Remove Hard Coded MyISAM requirement

5c5049b

tankerkiller125 approved these changes Nov 6, 2020

View reviewed changes

askvortsov1 requested changes Nov 7, 2020

View reviewed changes

tankerkiller125 self-requested a review November 8, 2020 14:23

tankerkiller125 suggested changes Nov 8, 2020

View reviewed changes

stale bot added the stale Issues that have had over 90 days of inactivity label Feb 9, 2021

stale bot removed the stale Issues that have had over 90 days of inactivity label Feb 9, 2021

ahosker commented Feb 10, 2021

View reviewed changes

luceos mentioned this pull request Mar 17, 2021

Web timeouts when installing through web installer #2258

Closed

tankerkiller125 suggested changes Apr 5, 2021

View reviewed changes

tankerkiller125 approved these changes Apr 6, 2021

View reviewed changes

askvortsov1 approved these changes Apr 6, 2021

View reviewed changes

luceos reviewed Apr 7, 2021

View reviewed changes

src/Install/DatabaseConfig.php Outdated Show resolved Hide resolved

lowercase null instead of NULL

d7b87ca

askvortsov1 approved these changes Apr 8, 2021

View reviewed changes

askvortsov1 reviewed Apr 8, 2021

View reviewed changes

StyleCI

8750ca0

askvortsov1 merged commit b1f166d into flarum:master Apr 9, 2021

askvortsov1 added this to the 0.1 milestone Apr 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove MyISAM Requirement #2442

Remove MyISAM Requirement #2442

ahosker commented Nov 6, 2020

tankerkiller125 left a comment

askvortsov1 left a comment

askvortsov1 Nov 7, 2020

ahosker Feb 10, 2021

askvortsov1 Nov 7, 2020

ahosker Feb 10, 2021

tankerkiller125 left a comment

luceos commented Nov 9, 2020 •

edited

Loading

stale bot commented Feb 9, 2021

tankerkiller125 commented Feb 9, 2021

askvortsov1 commented Feb 9, 2021

ahosker commented Feb 10, 2021 •

edited

Loading

ahosker left a comment •

edited

Loading

luceos commented Feb 10, 2021

tankerkiller125 commented Feb 10, 2021

askvortsov1 commented Mar 1, 2021

luceos commented Mar 1, 2021

askvortsov1 commented Mar 1, 2021

luceos commented Mar 6, 2021

tankerkiller125 left a comment

luceos commented Apr 6, 2021

tankerkiller125 commented Apr 6, 2021

askvortsov1 left a comment

askvortsov1 Apr 8, 2021

luceos Apr 9, 2021 •

edited

Loading

askvortsov1 Apr 8, 2021

askvortsov1 commented Apr 9, 2021

		@@ -31,7 +31,7 @@

		$table->unique(['discussion_id', 'number']);

		$table->engine = 'MyISAM';

Remove MyISAM Requirement #2442

Remove MyISAM Requirement #2442

Conversation

ahosker commented Nov 6, 2020

tankerkiller125 left a comment

Choose a reason for hiding this comment

askvortsov1 left a comment

Choose a reason for hiding this comment

askvortsov1 Nov 7, 2020

Choose a reason for hiding this comment

ahosker Feb 10, 2021

Choose a reason for hiding this comment

askvortsov1 Nov 7, 2020

Choose a reason for hiding this comment

ahosker Feb 10, 2021

Choose a reason for hiding this comment

tankerkiller125 left a comment

Choose a reason for hiding this comment

luceos commented Nov 9, 2020 • edited Loading

stale bot commented Feb 9, 2021

tankerkiller125 commented Feb 9, 2021

askvortsov1 commented Feb 9, 2021

ahosker commented Feb 10, 2021 • edited Loading

ahosker left a comment • edited Loading

Choose a reason for hiding this comment

luceos commented Feb 10, 2021

tankerkiller125 commented Feb 10, 2021

askvortsov1 commented Mar 1, 2021

luceos commented Mar 1, 2021

askvortsov1 commented Mar 1, 2021

luceos commented Mar 6, 2021

tankerkiller125 left a comment

Choose a reason for hiding this comment

luceos commented Apr 6, 2021

tankerkiller125 commented Apr 6, 2021

askvortsov1 left a comment

Choose a reason for hiding this comment

askvortsov1 Apr 8, 2021

Choose a reason for hiding this comment

luceos Apr 9, 2021 • edited Loading

Choose a reason for hiding this comment

askvortsov1 Apr 8, 2021

Choose a reason for hiding this comment

askvortsov1 commented Apr 9, 2021

luceos commented Nov 9, 2020 •

edited

Loading

ahosker commented Feb 10, 2021 •

edited

Loading

ahosker left a comment •

edited

Loading

luceos Apr 9, 2021 •

edited

Loading