SQL Upgrades need to be better

[rythos42]

The current upgrade process involves administrators copying files over top of their current installation, and needing to go to an upgrade.php script to upgrade their SQL. I feel like this is not the best way of doing it, since an admin can easily not realize they need to run the upgrade script and screw up their system.

A first-pass brainstorm would be to put the current system version in the database. Check that version against the running version, and if they are different, run the upgrade scripts.

We could have the upgrade-script just input the current version if it doesn't find it, allowing the initial bootstrap.

It may be worthwhile including the past upgrade scripts in this process so users coming from 0.8x don't have to deal with the upgrade script either.

[rythos42]

A few design notes:

• We have no idea what the initial version is before right now. The user could be upgrading from 0.75 for all we know. So if there isn't a version in the database, we can't do anything other than point the user to upgrade.php to deal with this problem manually.
• Only checking the manual upgrade if the user is a global admin. Likely won't let the automatic process run for regular users either, in case there is a problem.
• The upgrade from 097 to 101 now inserts a new table version into the database that will be used to track what version is installed, so we know more in the future.

From here, I want to check the existing version against the new version (so we'll need to store and update the new version whenever we do a release) and run any SQL upgrades that are in mysql_upgrade_queries.php.

I also need to look at the modules, because they have their own upgrade process that will need to be dealt with.

[rythos42]

Forgot $upgradeMsgs and $upgradeSettings variables - they were erroring. Also added a runIfTrue around the version insert so it only inserts if there is no value in the table. Future upgrades will need to update instead of insert.

Or maybe getCurrentDatabaseVersion grabs the largest value in the table, just to make sure.

[rythos42]

On WLeonards advice, using Table::createTable instead, to ensure only a single version entry.

Also, I think that the SQL upgrade process should only care about the "new" version, not the "old-new" version, so I changed the version to just be 101.

Added conditionals around the need for $upgradeMsgs and $upgradeSettings so future updates don't have to care about them.

[rythos42]

Database upgrades will now be done automatically...in version 1.2. 1.1 users will need to run upgrade.php, but they will be prompted to do so when global admins first log in.

Developers will need to be aware of how to treat database upgrades on thenafdev. I will write about this on the wiki.

[rythos42]

Added a check for future-proofing to ensure it doesn't quietly fail if $db_version isn't set.

[rythos42]

@TheNAF/contributors Just want to call attention to this Issue, since it affects future development work.

If you read nothing else, read the wiki on how to manage future database upgrades.

[mfranchetti]

We have no idea what the initial version is before right now. The user could be upgrading from 0.75 for all we know. So if there isn't a version in the database, we can't do anything other than point the user to upgrade.php to deal with this problem manually.

I haven't explicitly checked, but the upgrade procedures I have looked at are set to catch errors - ie, they only create tables/procedures/functions/columns if they are not already present. So in principle, it should be safe to apply an update procedure even if it is not required. if that can be confirmed (or implemented where it is required) then we can assume a default starting position of 0.75 and apply all subsequent upgrades. This shouldn't have any negative impacts on more recent installs and will ensure that all installs get brought up to the same level.

[rythos42]

This is what I wanted to do initially, and I think I just got it in my head that it was too complicated to check, but you're right -- I'll look and see if we can make this process cooler. :)

[rythos42]

I also need to account for systems that are several upgrades behind. So this Issue is long from over. :)

[rythos42]

Looking at this, I think the upgrade stuff is mostly ok. There's one line that's bugging me, if someone else could convince me it's ok:

UPDATE match_data SET mg = IF(f_player_id < 0, FALSE, IF(getPlayerStatus(f_player_id, f_match_id) = 1, FALSE, TRUE))

It just sets the mg flag based on the player status. In my local the set of players with getPlayerStatus <> 1 and the set of players with mg == 1 are the same. On thenaf, there are 985 players in the first set and 981 players in the second. So if we applied this line, 4 player match datas would get updated to have mg set to 1. Which...might be ok?

[thefloppy1]

Where else is this mg flag used?

On Thu, Jul 7, 2016 at 8:17 AM, Craig Fleming notifications@github.com wrote:

Looking at this, I think the upgrade stuff is mostly ok. There's one line that's bugging me, if someone else could convince me it's ok:

UPDATE match_data SET mg = IF(f_player_id < 0, FALSE, IF(getPlayerStatus(f_player_id, f_match_id) = 1, FALSE, TRUE))

It just sets the mg flag based on the player status. In my local the set of players with getPlayerStatus <> 1 and the set of players with mg == 1 are the same. On thenaf, there are 985 players in the first set and 981 players in the second. So if we applied this line, 4 player match datas would get updated to have mg set to 1. Which...might be ok?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/TheNAF/naflm/issues/130#issuecomment-230924099, or mute the thread https://github.com/notifications/unsubscribe/APXWhqXimetqworNyIai-beXJAb12ZF7ks5qTCmEgaJpZM4IqFZe .

[rythos42]

It's set to true when the player has an injury in a game. It's set to false if the player was in a match where they missed the match.

It's used in class_sqlcore somehow...updating mv_players if it's false?

It's used in class_stats, it looks like to tally how many matches the player has been in.

[rythos42]

There's a further problem (but less concerning) where I don't really want to scare 1.0 users by showing the upgrade message from 075-080 so I think we should just not show the upgrade message at all. This means current 0.75 users might miss some valuable information about manual SQL they need to run to tie teams to leagues.

I feel like this is an ok sacrifice to make. Unless someone has good ideas on how to get around it. :)

[thefloppy1]

this line here. (123)

$mstat_fields_suffix_player = "FROM matches,match_data WHERE $mstat_fields_suffix__common AND matches.match_id = match_data.f_match_id AND match_data.f_player_id = pid AND match_data.mg IS FALSE";

So it is a condition for statistical gain for the player, the syntax is correct, it sets default to false, which is fine. It looks messy but at the same time it is needed.

We can do a check before each addition to see if they are there, this way we can avoid scaring the crap out of old users of the software. This means that it is alot of work updating all the upgrades to get rid of the previous versions of upgrade button.

On Thu, Jul 7, 2016 at 8:36 AM, Craig Fleming notifications@github.com wrote:

It's set to true when the player has an injury in a game. It's set to false if the player was in a match where they missed the match.

It's used in class_sqlcore somehow...updating mv_players if it's false?

It's used in class_stats, it looks like to tally how many matches the player has been in.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/TheNAF/naflm/issues/130#issuecomment-230927880, or mute the thread https://github.com/notifications/unsubscribe/APXWhkv6gDHxe3XqlszN8LJQQtiJDQKRks5qTC3bgaJpZM4IqFZe .

[mfranchetti]

Looking at this, I think the upgrade stuff is mostly ok. There's one line that's bugging me, if someone else could convince me it's ok:

UPDATE match_data SET mg = IF(f_player_id < 0, FALSE, IF(getPlayerStatus(f_player_id, f_match_id) = 1, FALSE, TRUE))

So that's being run immediately after the column mg is added to the table match_data to populate it based on the current player status. Adding a WHERE mg IS NULL clause should ensure that the UPDATE only has an effect when the column in new. If data is pre-existing then it will be excluded from the update.

[rythos42]

That won't work, unfortunately. The column is added NOT NULL DEFAULT FALSE, so you can't tell the difference between a newly added column and an actually false value.

I wonder if you can put multiple statements into a single query. I know there's a ; operator that separates statements, but haven't used it in MySQL/PHP before.

[mfranchetti]

Ah, missed that. There's a couple of potential ways to resolve it, but I'd need to look at the calling functions more closely. Simplest might be to remove the NOT NULL DEFAULT FALSE from the ADD COLUMN statement, then UPDATE NULLS as required then add a third statement to ALTER TABLE and set the column to NOT NULL DEFAULT FALSE

[rythos42]

Let's do that.

Ran into another issue -- upgrade 075-080 requires the upgrading user to select whether they are using LRB5 or LRB5b/LRB6x. The automated process can't tell which, and I'm not certain how to (easily) resolve this.

[rythos42]

I implemented and tested the NULL column trick and it works perfectly. Did the same thing with another column played_0 for safety. I'd previously determined that letting it run as-is was ok, but better safe than sorry.

I think we're on the home stretch for this. I've got it running all upgrades to get to the current version. (It's a bit of a mess on first-load in 1.0!)

Just this LRB thing to think about. Do we have a list of all the external installs? I wonder if we could ask? Or we could make an assumption of LRB6 - possibly dangerous or possibly worthwhile. Is there something in the database we could check to tell?

We can also stop the upgrade process in the middle and ask the user to set a value in settings.php if we need to. It's not great, but I'm already doing it with $db_version, which is a new value required.

[rythos42]

We could also split the difference -- assume LRB6 but in the upgrade manual tell people to change the value if needed. Still possibly dangerous, since reading is hard, but it would likely make the process super easy for most installations.

[rythos42]

I went with the last idea for now. $db_upgrade_options are passed to the upgrade script, so admins can change this when they install 1.1.

Unless anyone has objections to this plan, or better ways around it, I think I'm done with this Issue (again :)). I'll leave it open for a little bit for discussion.

[rythos42]

Also, upgrade.php has been removed as it isn't necessary with this scheme. The 1.1 upgrade/release document should make note of this!