Constraints failed to build correctly in database.sql script. about opensourcepos

[jekkos]

I guess the conversion of your diff to migration should not be too hard. I'll try to solve the upgrade issue first on older MySQL. Once that works, I might look into the migration script if I find some time.

[jekkos]

Ok I see. Then we leave it there but we still need to port the person id changes to a migration but not run them in case they are already there

[objecttothis]

@SteveIreland I just reopened this. Maybe we can close it again, but I'm I'm trying to sort through the issues that @jekkos reported with the database migration failing on new installs. The reason for reopening it is because I was looking at the changes you made to constraints.sql and tables.sql. Maybe you did this already but something we need to think through is making sure that new installs (created from database.sql) and existing installs both of which are migrated using the migration scripts all end up with the same schema.

So, for example, you changed the ospos_customers table creation script to use person_id as the primary key. Databases created before this change will have a different schema, will they not? I think we need a migration script to make these changes rather than make them in the tables.sql and constraints.sql. The exception to this rule is if the database.sql script simply won't run. In that case we modify the tables.sql and constraints.sql and we create a migration script which changes the schema for existing installs.

A secondary problem is syntax.

The proper format for declaring the primary key is PRIMARY KEY ([COLUMN NAME]) (https://www.w3schools.com/sql/sql_ref_primary_key.asp)

So I guess my question is "Can any or all of the changes you made in constraints.sql and tables.sql be reverted and replaced with a migration script?" I'm working on fixing the problems in the migration scripts for new installs. Primarily they are problems caused in 20210422000000_database_optimizations.php.

[objecttothis]

Thanks @SteveIreland . It sounds like they introduced this as a breaking change feature in 8.4.0 then didn't deprecate or document it. SNAFU. That said it sounds like going forward we can expect it to be broken on future versions of MySQL so I agree that the tables.sql and constraints.sql need to be modified just enough to not get these errors. This also means that any existing migrations which have anything to do with changes we are making to the SQL may need to be modified to account for both scenarios. For example if the changes you make change constraint 'foo' then anything we do in migrations related to whatever foo was before needs to account for both possibilities. Further we need a migration script to make identical changes on existing databases. That migration needs to have checks that don't run commands which make changes that have already been made.

[SteveIreland]

The corrections were committed, and I was able to build the project, complete the build of the version 8 database using the generated database.sql and when I logged in I didn't encounter any migration step issues. So, I'm flagging this as complete.

[jekkos]

Very cool thanks for this contribution @SteveIreland !

[jekkos]

I will upgrade the docker also to the newer MySQL version. Also we can update the docs and mention this supported version.

[jekkos]

@SteveIreland we should not forget migration scripts to apply same changes to existing installs.

[jekkos]

I have rewritten the commits in master branch as I'd like to keep a linear history. A local merge made commit made it in there which sort of 'pollutes' history graph.

Be careful and move any new work you have directly on master to a new branch first before you pull again, otherwise it might be overwritten.

[SteveIreland]

Yeah, I saw it but didn't want to make it worse. Sorry about that. Thanks for cleaning it up for me.

[SteveIreland]

I'll be working on the migration scripts from an existing database very soon. But you've distracted me with the docker setup and now I've decided to migrate my client to the new system targeting docker on a Ubuntu hosted VPS. That requires me to learn one or two new things first. :-)

[jekkos]

Cool to hear you're picking up some new tricks. I have changed the dev server to point to master again. Indeed currently there is an issue running the db init, looked into this briefly yesterday. It does not seem to be related to the changes you did at first sight.

It's the attribute links unique key that is duplicated for some reason.

[SteveIreland]

If you don't get to it I'll put together a pull request for it this weekend.

[SteveIreland]

After further research, it appears that I need to add a migration script that only corrects the ospos_sales_items_taxes_ibfk_1 constraint. It appears that the other changes to the tables themselves were taken care of during other migration scripts which are already part of the upgrade. And the "suspended" tables were dropped as part of the normal migration scripts - so no worries there. The change to the originating tables.sql and constraint.sql are still needed when doing an initial install in MySQL versions >= 8.

[jekkos]

Ah wait I might have overseen this, but indeed we always run all migrations after database.sql. I believe the initial script needs to be 'frozen' and new changes should go in a migration only. This way there is no difference between a clean install and an upgrade from any version to the latest, they go through the same upgrade steps and run migrations one by one.

So perhaps we need to revert the changes to database.sql and make sure all changes are in the migration file?

[SteveIreland]

Nope. "after database.sql" is the show stopper. The changes MUST be made to the table.sql and constraints.sql.

The generated database.sql script is what needs to be able to run to completion by itself - in a MySQL 8.x environment - first before the migration can start.

Since the constraints were bad and the person_id keys on some tables were badly defined it was crashing.

[SteveIreland]

This was mentioned earlier, somewhere else, but the migration steps had already made the appropriate corrections to the person_id. The only issue I stumbled across (post-migration) was just one foreign key constraint that was incorrect. The correction was added to the script and was tested. I'll be retesting this probably three or more times, but it appears to be working fine.

[objecttothis]

Nope. "after database.sql" is the show stopper. The changes MUST be made to the table.sql and constraints.sql.

The generated database.sql script is what needs to be able to run to completion by itself - in a MySQL 8.x environment - first before the migration can start.

Since the constraints were bad and the person_id keys on some tables were badly defined it was crashing.

I see here that you are answering part of my question. The problem I'm confused with is what exactly was breaking when running database.sql prior to these changes. More specifically what the errors are. If indeed database.sql won't run in a version, then tables.sql and constraints.sql need to be minimally modified. The thing that is strange for me is that I wasn't getting errors running database.sql in MySQL 8.x. That's not to say you weren't, but just that I think it will help for me to understand under what circumstances database.sql wouldn't run as it was.

[objecttothis]

I just ran the database.sql from 3.3.9 on an empty database in MySQL 8.2.0. @SteveIreland does that database.sql work for you? I ask because it will be helpful to know at what point MySQL 8 stops working properly for us.
database.zip

[SteveIreland]

I just ran the database.sql file from a copy of 3.3.9 that I have in a fresh empty database in MySQL 8.4.0 and received the following error. There is a subsequent one coming, but the first failure is always on the misconfiguration of the person_id.

[objecttothis]

I wonder if this is a difference between MySQL 8.4.0 and 8.2.0. I'll try to recreate in the morning. If we do find that it's an incompatibility then we would need to make the necessary changes to constraints.sql and tables.sql but we also need a migration script which will make identical changes to existing databases so that we don't end up with people having different schemas.

[SteveIreland]

I'm working out of a hotel room at the moment, but it appears that 8.4 changes the default based on the 8.4 problem report.

https://bugs.mysql.com/bug.php?id=114838

Otherwise, if I were at home then I'd confirm it the hard way by installing the previous MySQL version. :-)

[dch90]

So I guess my question is "Can any or all of the changes you made in constraints.sql and tables.sql be reverted and replaced with a migration script?" I'm working on fixing the problems in the migration scripts for new installs. Primarily they are problems caused in 20210422000000_database_optimizations.php.

Hi all, I am very new to this great project, and as I was attempting to build the project from scratch I ran into this exact issue.

The latest migration script expects a person_id index to be present on ospos_customers, ospos_employees, and ospos_suppliers table. but these indices were removed directly from tables.sql file.

I do agree with @objecttothis that these types of changes need to happen via migration scripts to allow backwards compatibility.

I do wonder, how come these changes were committed directly to master branch without going through PR's and reviews.

Edit: it is late and I failed to read the entire issue closely. If the change is required due to a breaking change on MySQL, how feasible is it to update the latest migration file to remove the statements where 'person_id' indices are dropped

[objecttothis]

So I guess my question is "Can any or all of the changes you made in constraints.sql and tables.sql be reverted and replaced with a migration script?" I'm working on fixing the problems in the migration scripts for new installs. Primarily they are problems caused in 20210422000000_database_optimizations.php.

This script was working when created, but due to unavoidable changes, it needs adjustments to be properly compatible. @SteveIreland is working on fixes for it in a PR now. @SteveIreland what does your timeline look like for that PR?

Hi all, I am very new to this great project, and as I was attempting to build the project from scratch I ran into this exact issue.

Welcome!

The latest migration script expects a person_id index to be present on ospos_customers, ospos_employees, and ospos_suppliers table. but these indices were removed directly from tables.sql file.

I do agree with @objecttothis that these types of changes need to happen via migration scripts to allow backwards compatibility.

The exception will be when a new instance is created using database.sql won't run as we are finding out with MySQL 8.4.x and up. In that case tables.sql and constraints.sql need to be modified as minimally as possible and in such a way that is still backward compatible with previous versions of MySQL. I believe that is what @SteveIreland has done here. What does need to be done however is existing migration scripts which will not run on MySQL 8.4.x also need to be modified so that they are compatible with both AND new migrations need to be made so that existing install schemas match the one created by database.sql. It's a hot mess that in every other case is avoided by not touching tables.sql or constraints.sql but only using migration scripts. Unless we are missing something, this can't be avoided here.

I do wonder, how come these changes were committed directly to master branch without going through PR's and reviews

I think the changes were made in the ci4-upgrade branch directly before the entire thing was merged into the master. Unfortunately that guarantees that some things will have been missed. Moving forward though, it's the master so everything is required to go through PR and therefore code review. @SteveIreland has a PR he's working with now to fix this issue.