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

add mysql row format dynamic option #18002

Merged
merged 19 commits into from Sep 23, 2021
Merged

add mysql row format dynamic option #18002

merged 19 commits into from Sep 23, 2021

Conversation

peterhashair
Copy link
Contributor

@peterhashair peterhashair commented Sep 15, 2021

Description:

Add row format equals dynamic as default, regarding the large text field for creating table failure.

Review

@peterhashair peterhashair added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Sep 15, 2021
@peterhashair peterhashair added this to the 4.5.0 milestone Sep 15, 2021
@peterhashair peterhashair linked an issue Sep 15, 2021 that may be closed by this pull request
@peterhashair peterhashair changed the title remove html changes add mysql row format dynamic as default Sep 15, 2021
add key not exist
core/Db/Settings.php Outdated Show resolved Hide resolved
update the database change on the Core Updates
config/global.ini.php Outdated Show resolved Hide resolved
core/Updates/4.5.0-b1.php Outdated Show resolved Hide resolved
config/global.ini.php Outdated Show resolved Hide resolved
Peter Zhang added 2 commits September 16, 2021 17:06
convert naming from disable to enable, add comment, update update, take 3 params.
There is an extra space on create sql, remove the ending space, if there is any. to Pass test.
@peterhashair peterhashair changed the title add mysql row format dynamic as default add mysql row format dynamic option Sep 16, 2021
update some description
Copy link
Member

@tsteur tsteur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sgiehl I'm not quite sure what we do during the installation. Are we using utf8mb4 by default when possible? Would we need to do anything during the installation so users benefit from this?

config/global.ini.php Outdated Show resolved Hide resolved
core/Updates/4.5.0-b1.php Outdated Show resolved Hide resolved
@peterhashair peterhashair added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. and removed not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. labels Sep 16, 2021
Peter Zhang and others added 3 commits September 17, 2021 12:11
update global wording and remove dynamic on in 4.5.0-b1
add createTable dynamic test
@tsteur
Copy link
Member

tsteur commented Sep 19, 2021

@sgiehl be great to let us know if you know maybe whether we need to do anything on installation re this setting

@sgiehl
Copy link
Member

sgiehl commented Sep 19, 2021

Yes. We need to use a default that works for all users and change the value on installation. Them same way it's done for utf8mb4

Peter Zhang added 2 commits September 20, 2021 11:23
add migration row format dynamic
check when installation database is utf8mb4 format then auto enable  dynamic row format
@peterhashair
Copy link
Contributor Author

@tsteur Updated the Installation script, but not sure about the ms sql option, I set it false for now.

core/Db/Settings.php Outdated Show resolved Hide resolved
update dynamic to default when utf8mb4 is set
Copy link
Member

@tsteur tsteur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, we're getting there. Looks a lot cleaner without the config 👍 sorry for having to remove it but it'll simplify things in the future 🚀

core/Db/AdapterInterface.php Outdated Show resolved Hide resolved
core/Db/Schema/Mysql.php Outdated Show resolved Hide resolved
core/DbHelper.php Outdated Show resolved Hide resolved
core/Updates/4.5.0-b2.php Outdated Show resolved Hide resolved
…ation updates

remove uft8 function, set row format as a quick function, remove migration updates
@peterhashair
Copy link
Contributor Author

@tsteur Sorry about the issue being dragging too long, I should scope it a little bit better at the beginning.

@tsteur
Copy link
Member

tsteur commented Sep 22, 2021

@peterhashair all good, the problem was we didn't scope the issue well enough in the actual issue. We'll do better :) And good we found that we can simplify it all that's great.

Peter Zhang added 2 commits September 22, 2021 20:56
# Conflicts:
#	core/Db/Adapter/Pdo/Mssql.php
#	core/Db/Adapter/Pdo/Pgsql.php
resolve conflicts
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left a minor comment. Otherwise looks good to me

core/Db/Schema/Mysql.php Outdated Show resolved Hide resolved
remove rtrim obsolete
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me know

@tsteur tsteur merged commit 8b14a4e into 4.x-dev Sep 23, 2021
@tsteur tsteur deleted the m-16834-row-format-mysql branch September 23, 2021 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use row_format=dynamic by default when creating a table
3 participants