-
Notifications
You must be signed in to change notification settings - Fork 47
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
Collection of fixes for the update script #643
Conversation
About 1.: I added a rigorous To ensure the operation to be safe (having no TOCTOU problem) I enclosed the two queries ( To keep the statement blocks readable I reformatted them and put the statements in variables instead of noting them inside the function calls.
|
Transactions does not work with tables of type MyISAM. Because of that we changed the table type of the existing tables to InnoDB which supports transactions. But this is at a later point of the update script during the update to 2.4.99.2 or 2.4.99.3. At the point, where I want to (re)-create the new tables (update to 2.4.99.0) I can not be sure about the table type of the previously created tables. Normally I would simply assume, that the previously created tables for the 2.5-branch are new enough to fall under "modern" default settings of a MySQL or MariaDB-server (default charset UTF-8, default table type InnoDB) because all servers I have access to are cofigured that way. But at the end I can not be sure about this. So my question is, if we can reorder the database queries for changes of the database structure in the update procedure without breaking anything. IMHO it should be possible to reorder at least the changes of the several pre releases (2.4.99.x) without breaking anything. But I am not sure. @loesler Can you please also take a look in the update script to find possible pitfalls? Thank you. |
I'm sorry, I don't really understand the problem of your post. Do you like to know the
Having such a list, it is possible to check the table type (and to adapt the type if required). |
I think about changing the position of the ALTER-statements for changing the table type to the start of the database operations, directly behind creating the db_settings.php. But writing this, I think, this is a bad idea and unnecessary. There is completely no TOCTOU problem at this point. I can delete a possibly existing table and create it afterwards again without any problem without a transaction (that would require a proper table type). I will change the code again. Thank you for the input. |
…ing them again This fixes issues with upgrades of forum scripts from 2.4.x-versions to any 2.5-version, when there was a testing installation of a 2.5-development version, that created these tables before. Often such testing installations with a following downgrade to a stable version results in remaining, orphaned tables, that breaks the update script in a later run, when it tries to create the tables again. I put the queries to drop and create the tables into a transaction. Thatswhy I needed to introduce the function mysqli_multi_query to run all four queries per table at once.
Dropping tables, that should not exist in this moment can be handled independently from the creation process of the tables.
…et of the table The first repeat of setting of the column size (varchar(255)) is necessary to make the rest of the query (CHARSET SET and COLLATE) working. The second query in the update procedure ensures the correct charset for the case of an update starting from version 2.4.99.1.
…interrupt the update in this case
Looks comprehensible to me. |
I will merge the PR and will perform further tests afterwards. I've tested the changes with an update from a stable version of the 2.4.x branch. I tested the queries for not breaking in case of a repeated change (in example: setting the field size of the e-mail-column to 255 thrice). But these tests were carried out under laboratory conditions for every of the queries (with a PHP-script with the code for changing the table definitions only) and not in a existing forum that was previously upgraded to an intermediate release version. Nevertheless I don't expect the queries to break anything. |
Currently there are a few new threads with error reports about the update script in the project forum. We can identify three main issues.
UNIQUE
without checking the real values in the database table for doublettes breaks the update script and leaves the database in an intermediate state. Some changes were made, others not. Check for doublettes before trying to alter the definition of the column.This PR should address these problems.