nunomaduro / laravel-optimize-database

💨 Provides a good starting point for production-ready SQLite databases
https://laravel-optimize-database.vercel.app
MIT License
220 stars 7 forks source link

Error running a "fresh" migration after using package #8

Open upayvitorenes opened 1 month ago

upayvitorenes commented 1 month ago

After - according to the doc's instructions - executing:

php artisan db:optimize and php artisan migrate

and checking that indeed there is a difference/gain in performance, I wanted to repeat the process without the "package influence" to better benchmark the performance impact, so I ran:

php artisan migrate:fresh

and got this thrown to the console:

Dropping all tables .................................................................................................................. 7.56ms DONE

   Illuminate\Database\QueryException 

  SQLSTATE[HY000]: General error: 11 database disk image is malformed (Connection: sqlite, SQL: select name from sqlite_master where type = 'table' and name not like 'sqlite_%' order by name)

  at vendor/laravel/framework/src/Illuminate/Database/Connection.php:825
    821▕                     $this->getName(), $query, $this->prepareBindings($bindings), $e
    822▕                 );
    823▕             }
    824▕ 
  ➜ 825▕             throw new QueryException(
    826▕                 $this->getName(), $query, $this->prepareBindings($bindings), $e
    827▕             );
    828▕         }
    829▕     }

      +42 vendor frames 

  43  artisan:13
      Illuminate\Foundation\Application::handleCommand(Object(Symfony\Component\Console\Input\ArgvInput))

I still could run php artisan migrate and it did its thing, but I had to delete the database.sqlite file, and re-create one to "regain control" of the DB. If this is expected after the optimization is done, maybe it's worth mentioning in the docs - maybe it does not make sense to run the migrate:fresh command after?

JohnONolan commented 3 weeks ago

Had the same issue! Would love to understand if there's a different flow that should be used for local dev

I also found that, after removing the migration, migrate:fresh would produce bus errors on every 2nd run. Had to remove the package completely and dump caches to resolve this

wilsenhc commented 3 weeks ago

I ran into the same issue, any recommendations on this @nunomaduro?

edgrosvenor commented 3 weeks ago

The published migration has to run before any other migrations are run, so all you need to do is change its name so that it sits above the rest of your migrations and you should be fine.

upayvitorenes commented 3 weeks ago

The published migration has to run before any other migrations are run, so all you need to do is change its name so that it sits above the rest of your migrations and you should be fine.

Good catch. Should be handled by the package, thou - shouldn't it?

edgrosvenor commented 3 weeks ago

It only half works. @JohnONolan's observation that the fresh migration fails every other run is a different bug that isn't solved by simply renaming the migration. I'm pulling the package out of my project for now and will keep an eye on its progress. I don't know enough about this stuff to be really helpful unfortunately.

edgrosvenor commented 3 weeks ago

This is probably the cause of the error on every second run of php aritsan fresh: https://github.com/laravel/framework/discussions/53044

I'm not sure how this package will work around that without tweaking how Laravel itself handles the database connection in that command. There is information in that discussion, however, about how to work around it.

edgrosvenor commented 3 weeks ago

@nunomaduro I've done a bunch of experimenting here and I'm not sure any workaround I find can get around the fact that Laravel simply empties out the sqlite database file when php artisan fresh is run. Do you suppose that Taylor is open to a PR to change that behavior or is there maybe something happening internally to address this? I know there's a discussion (had been an issue) that I linked to above so I know that they're aware of the problem.

It's not a blocker for me to be able to use the package because I have my own php artisan fresh command that refreshes the database and I never run php artisan migrate:fresh directly. So I can simply change my own logic to drop all the tables, indexes, etc. rather than calling migrate:fresh and blanking out the file. But without changing the framework's behavior when that command is run, I think the perception that this package is "buggy" is going to be pretty persistent.

edgrosvenor commented 6 days ago

A quick update for anyone interested. In my own project's workflow, I simply replaced php artisan migrate:fresh with php artisan migrate:refresh and it works fine. The only downside, I think, is that there are lots of people who have joined the "never write a down migration" movement, so for them this isn't going to be a good option. But if your down migrations will fully tear your database down, then you can go this route and the entire package seems to work as advertised.