New Feature: allow choosing innoDb vs MyIsam engine for MYSQL during installation #1043

TonisOrmisson · 2018-04-17T07:03:13Z

Formerly opened form master #977, now for develop

Some notes:
in order for innoDb to work one needs:

innodb_large_prefix = 1
innodb_file_format = Barracuda;
innodb_file_format_max = Barracuda;

otherwise you will get the "Error 1071: Specified key was too long; max key length is 767 bytes" for the text indexes, (Error 1071: Specified key was too long; max key length is 767 bytes)

I enabled this for travis
https://github.com/LimeSurvey/LimeSurvey/compare/develop...TonisOrmisson:develop-innodb?expand=1#diff-354f30a63fb0907d4ad57269548329e3R45

and checking of this will be done in installer validation here
https://github.com/LimeSurvey/LimeSurvey/compare/develop...TonisOrmisson:develop-innodb?expand=1#diff-12a8fc159f48cb97e97609f6c43aab0aR258

Did some major changes in the installer logic.
Why? because I needed to check the Innodb settings status in the model validation: so the dbConnection needed to move from the controller to the Model. And so all of the connection related things were moved to model. Controller got much more cleaner and is now not doing things that logically should not be there.

Also cleaned up the view along that since there was code that should not be in the view.

Tests:
I enabled separate set of tests for InnoDB and MyIsam. So the whole test set runs both ways now.
Also added PHP 7.2 to the matrix.

... I think we should also use the pgsql for testing on travis. As far as I understand this is also included for travis so I would use an ENV DBTYPE to run test matrix also on available non-mysql options.

I have tested this manually also during the work, but since there is a lot of changes here I would ask for you to review the code as well as also help to test manually. Especially for MSSQL that I do not have and is not tested against!

…rol-label' is auto-generated

TonisOrmisson · 2019-03-22T08:27:26Z

Ok now.
The timings and responses tables are created by the SurveyActivator using the same session. So the timigs table was OK. But tokens table was created with a separate request. This is now fixed also. Also added the engine option to activate_helper and AuditLog table creations. I tested the activation parts wihle changing the engines with responses/timings/tokens table. Feel free to merge.

Shnoulle · 2019-03-22T09:09:48Z

plugins/AuditLog/AuditLog.php

@@ -557,6 +557,11 @@ public function beforeActivate()
        {
            if (!$this->api->tableExists($this, 'log'))
            {
+                $options = '';
+                if (Yii::app()->db->driverName == 'mysqli' || Yii::app()->db->driverName == 'mysql') {


I think more of something in API ;) : api->createTable function (but this api is … so … so … light)

dominikvitt · 2019-04-03T11:13:35Z

I just finished with testing.
There is occasionally a following error when trying to import .lsa file:

Call to a member function getColumnNames() on null
application/helpers/admin/import_helper.php(2205)

There is also this error when trying to activate a survey with many columns:

CDbCommand failed to execute the SQL statement: SQLSTATE[42000]: Syntax error or access violation: 1118 Row size too large (> 8126)

It is obvious those errors are related to innoDB limitation of having too many columns.

In such case, import/activation should be aborted and warning should be shown to user.
Anybody have some idea about this?

Shnoulle · 2019-04-03T11:34:52Z

It is obvious those errors are related to innoDB limitation of having too many columns.

InnoDB don't send exception here ?

LimeSurvey/application/helpers/admin/activate_helper.php

Lines 457 to 466 in 99d396d

    
           try { 
        
               Yii::app()->db->createCommand()->createTable($sTableName, $aTableDefinition); 
        
               Yii::app()->db->schema->getTable($sTableName, true); // Refresh schema cache just in case the table existed in the past 
        
           } catch (CDbException $e) { 
        
               if (App()->getConfig('debug')) { 
        
                   return array('error'=>$e->getMessage()); 
        
               } else { 
        
                   return array('error'=>'surveytablecreation'); 
        
               } 
        
           }

Shnoulle · 2019-04-03T11:37:08Z

Just checkout :

CDbCommand failed to execute the SQL statement: SQLSTATE[42000]: Syntax error or access violation: 1170 BLOB/TEXT column 'firstname' used in key specification without a key length. The SQL statement executed was: ALTER TABLE lime_participants CHANGE firstname firstname text NOT NULL

But seems all my setup wads broken now … maybe an issue in DBversion …

TonisOrmisson · 2019-04-03T11:40:02Z

Thanks for testing!
Can you tell whether the InnoDb acts differently compared to MyIsam when the column limit is reached? I would think that such error would be regardless of engine?

Maybe you can add some test files to the tests/data/surveys folder so that this could be tested automatically. Eg a survey with too much columns for MyIsam. And a survey with too much columns for InnoDb.

Did you try with debugging = 0, was it the same?

dominikvitt · 2019-04-03T11:42:06Z

When activating, it shows nice warning message when debug=0.

dominikvitt · 2019-04-03T11:49:04Z

Column limit does not apply to the BLOB and TEXT type.
In current development version, all response columns are created as TEXT (because data encryption feature require it).
So, I'll try to merge your changes to my local develop version and see if problem is corrected.

TonisOrmisson · 2019-04-03T11:49:10Z

@dominikvitt
I guessing it is the same behaviour that we have currently (in master) and with MyIsam. If that's the case, maybe its not specifically related to this feature? If you turn off debugging there is a number of places you might end up with an exception thrown.

TonisOrmisson · 2019-04-03T11:51:29Z

@Shnoulle can you check what is the status of the engine variables (see top of page):

innodb_large_prefix;
innodb_file_format;
innodb_file_format_max;

your issue sounds like related to innodb_large_prefix.
This should be checked during installation actually. Or maybe you changed some after installation?

Shnoulle · 2019-04-03T12:11:41Z

I just checkout my current develop with your's (i want to test set innodb after install too)

Currently : even with core develop : i have issue with my master DB to upgrade to develop.
@dominikvitt : do you know when we check DB updater ?

dominikvitt · 2019-04-03T12:19:35Z

I would test now upgrading from master to develop.

Shnoulle · 2019-04-03T12:42:15Z

I would test now upgrading from master to develop.

Ok , i check too (maybe must do a checkintegrity , i test and if checkintegrity fix my issue i report to do one before upgrade (in fact i think we must do one each time we upgrade : new Feature …)

dominikvitt · 2019-04-03T13:27:02Z

@Shnoulle Database problem is fixed now, you can try again.

Shnoulle · 2019-04-03T13:47:53Z

@dominikvitt , sorry : https://bugs.limesurvey.org/view.php?id=14729 :)

dominikvitt · 2019-04-03T15:39:49Z

Current develop version is already using TEXT db fields for all response columns, so we can't solve it like that. Let it stay for now as it is.

@TonisOrmisson Can you add check to see if table is actually created while importing .lsa file?
If table is not created, then you can show the same message as it is for survey activation.

TonisOrmisson · 2019-04-04T06:21:59Z

created a new survey with this branch, activated and created some responses, exported lsa and imported as new survey. All relevant tables were created. responses, timings, tokens.

maziminke · 2019-04-04T07:10:21Z

In current development version, all response columns are created as TEXT (because data encryption feature require it).
Does that mean that e.g. for the date question type or the numeric input a TEXT field is used as well?

How does that work with features like filtering statistics by date or calculating e.g. mean or average values at statistics and other features? Don't they rely on a proper DB field type?

Shnoulle · 2019-04-04T07:40:19Z

In current development version, all response columns are created as TEXT (because data encryption feature require it).

Really ? And this was merged in develop … for alpha version … the DB column type is the most complicated system to move to QuestionObject … don't do it at start of devlopment seem realy an issue in my opinion …

dominikvitt · 2019-04-05T11:30:09Z

How does that work with features like filtering statistics by date or calculating e.g. mean or average values at statistics and other features? Don't they rely on a proper DB field type?

All features you mentioned don't rely on specific DB field type and data would be read the same regardless of DB field type. So, there shouldn't be any different behaviour when using TEXT DB field type.

dominikvitt · 2019-04-05T11:43:37Z

created a new survey with this branch, activated and created some responses, exported lsa and imported as new survey. All relevant tables were created. responses, timings, tokens.

Please try to create a survey containing large number of response columns (e.g. over 100 or 200) and redo all steps to test it.
Then you should be able to reproduce that error.

TonisOrmisson · 2019-04-05T11:50:04Z

Maybe you can add some test files to the tests/data/surveys folder so that this could be tested automatically. Eg a survey with too much columns for MyIsam. And a survey with too much columns for InnoDb.

Shnoulle · 2019-04-05T12:56:34Z

Maybe you can add some test files to the tests/data/surveys folder so that this could be tested automatically. Eg a survey with too much columns for MyIsam. And a survey with too much columns for InnoDb.

A survey with 10 000 column must broke for all DB ;) (else : tell me the DB to be used for such system)

TonisOrmisson added 30 commits January 3, 2018 17:15

add dbengine to InstallerConfigurationForm

72e3ccf

DEV add InnoDB support: installer: getDbEngines

e10438b

DEV add InnoDB support: installer: label comes from Model!

0303297

DEV add InnoDB support: installer: 'required' comes from Model! 'cont…

81c7959

…rol-label' is auto-generated

DEV add InnoDB support: installer: form JS

8923a65

DEV add InnoDB support: installer: engine validation

2754791

DEV add InnoDB support: installer: setMySQLDefaultEngine

06121c6

DEV add InnoDB support: remove ALL references to engines

4d0af89

DEV add InnoDB support: make engine selection work

cc6575a

DEV add InnoDB support: make engine selection work

9c466b8

SHOW ALL ERRORS (INSTALLATION DEBUG)

d5abf2b

relocate dbEngine

142341d

relocate dbEngine

4f80d36

typo

2785577

Merge commit '72e3ccf' into develop-innodb

8d0a4fe

Merge commit 'e10438b' into develop-innodb

8882293

Merge commit '0303297' into develop-innodb

4fd63c8

Merge commit '81c7959' into develop-innodb

e1e9c90

Merge commit '8923a65' into develop-innodb

a8bf4be

Merge commit '2754791' into develop-innodb

513d4c3

Merge commit '06121c6' into develop-innodb

6a96c09

Merge commit '4d0af89' into develop-innodb

5f1749e

Merge commit 'cc6575a' into develop-innodb

7cc06be

Merge commit '9c466b8' into develop-innodb

1372fd7

Merge commit 'd5abf2b' into develop-innodb

919000b

Merge commit '142341d' into develop-innodb

62ff3b4

Merge commit '4f80d36' into develop-innodb

397c31d

Merge commit '2785577' into develop-innodb

f74fd7b

DEV: innodb support: test change travis config

c572c8b

DEV: innodb support: shorten surveyls_title

7a97bee

TonisOrmisson added 2 commits March 22, 2019 10:19

fix mysql db engine for activate_helper (is it used?)

1c663c5

fix mysql db engine for AuditLog

5e24ba2

TonisOrmisson changed the title ~~New Feature: allow choosing innoDb vs MyIsam engine for MYSQL during installation (WIP)~~ New Feature: allow choosing innoDb vs MyIsam engine for MYSQL during installation Mar 22, 2019

Shnoulle reviewed Mar 22, 2019

View reviewed changes

merge upstream

1116599

Merge remote-tracking branch 'upstream/develop' into develop-innodb

c330470

Merge branch 'develop' into develop-innodb

d653fea

dominikvitt merged commit 4f97fb0 into LimeSurvey:develop Apr 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Feature: allow choosing innoDb vs MyIsam engine for MYSQL during installation #1043

New Feature: allow choosing innoDb vs MyIsam engine for MYSQL during installation #1043

TonisOrmisson commented Apr 17, 2018

TonisOrmisson commented Mar 22, 2019

Shnoulle Mar 22, 2019

dominikvitt commented Apr 3, 2019

Shnoulle commented Apr 3, 2019 •

edited

Loading

Shnoulle commented Apr 3, 2019 •

edited

Loading

TonisOrmisson commented Apr 3, 2019

dominikvitt commented Apr 3, 2019

dominikvitt commented Apr 3, 2019

TonisOrmisson commented Apr 3, 2019

TonisOrmisson commented Apr 3, 2019

Shnoulle commented Apr 3, 2019 •

edited

Loading

dominikvitt commented Apr 3, 2019

Shnoulle commented Apr 3, 2019

dominikvitt commented Apr 3, 2019

Shnoulle commented Apr 3, 2019

dominikvitt commented Apr 3, 2019

TonisOrmisson commented Apr 4, 2019

maziminke commented Apr 4, 2019

Shnoulle commented Apr 4, 2019

dominikvitt commented Apr 5, 2019

dominikvitt commented Apr 5, 2019

TonisOrmisson commented Apr 5, 2019

Shnoulle commented Apr 5, 2019 •

edited

Loading

New Feature: allow choosing innoDb vs MyIsam engine for MYSQL during installation #1043

New Feature: allow choosing innoDb vs MyIsam engine for MYSQL during installation #1043

Conversation

TonisOrmisson commented Apr 17, 2018

TonisOrmisson commented Mar 22, 2019

Shnoulle Mar 22, 2019

Choose a reason for hiding this comment

dominikvitt commented Apr 3, 2019

Shnoulle commented Apr 3, 2019 • edited Loading

Shnoulle commented Apr 3, 2019 • edited Loading

TonisOrmisson commented Apr 3, 2019

dominikvitt commented Apr 3, 2019

dominikvitt commented Apr 3, 2019

TonisOrmisson commented Apr 3, 2019

TonisOrmisson commented Apr 3, 2019

Shnoulle commented Apr 3, 2019 • edited Loading

dominikvitt commented Apr 3, 2019

Shnoulle commented Apr 3, 2019

dominikvitt commented Apr 3, 2019

Shnoulle commented Apr 3, 2019

dominikvitt commented Apr 3, 2019

TonisOrmisson commented Apr 4, 2019

maziminke commented Apr 4, 2019

Shnoulle commented Apr 4, 2019

dominikvitt commented Apr 5, 2019

dominikvitt commented Apr 5, 2019

TonisOrmisson commented Apr 5, 2019

Shnoulle commented Apr 5, 2019 • edited Loading

Shnoulle commented Apr 3, 2019 •

edited

Loading

Shnoulle commented Apr 3, 2019 •

edited

Loading

Shnoulle commented Apr 3, 2019 •

edited

Loading

Shnoulle commented Apr 5, 2019 •

edited

Loading