-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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
[8.x] Changing the dump and restore method for a PostgreSQL database #34293
Conversation
@@ -73,7 +73,7 @@ protected function schemaState(Connection $connection) | |||
*/ | |||
protected function path(Connection $connection) | |||
{ | |||
return tap($this->option('path') ?: database_path('schema/'.$connection->getName().'-schema.sql'), function ($path) { | |||
return tap($this->option('path') ?: database_path('schema/'.$connection->getName().'-schema.dump'), function ($path) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The result obtained with ** pg_dump -Fc ** is not SQL therefore not readable like classic SQL ...
I wasn't able to reproduce this issue so don't feel comfortable changing the code at this time. Need more feedback from Pgsql users as to specifically what is happening. |
No problem |
PgSQL user here. I see no problem with the PR and in fact would also argue it's more idiomatic to use |
@mfn @d-stephane ever since this PR Postgres schema dump has been totally broken to be honest. First, the migration data is never stored in the dump because this entire method no longer works: It doesn't work because obviously this binary dump doesn't include SELECT statements it can parse out. This causes the subsequent migrations to break because Laravel thinks it always needs to reload the state because the migrations table is empty. |
I have created a new PR to try to fix this: #35018 |
I just tested, it works very well :) |
PR related to issue #34281