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

PostgreSQL: Fix db:structure:load silent failure on SQL error #24773

Merged
merged 1 commit into from Jun 17, 2016

Conversation

Projects
None yet
7 participants
@ralinc
Contributor

ralinc commented Apr 28, 2016

PostgreSQL: Fix db:structure:load silent failure on SQL error

The command line flag "-v ON_ERROR_STOP=1" should be used when invoking psql to make sure errors are not suppressed.

Example: psql -v ON_ERROR_STOP=1 -q -f awesome-file.sql my-app-db

Fixes #23818.

@rails-bot

This comment has been minimized.

Show comment
Hide comment
@rails-bot

rails-bot Apr 28, 2016

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @kaspth (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

rails-bot commented Apr 28, 2016

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @kaspth (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@ralinc

This comment has been minimized.

Show comment
Hide comment
@ralinc

ralinc Apr 28, 2016

Contributor
Contributor

ralinc commented Apr 28, 2016

PostgreSQL: Fix db:structure:load silent failure on SQL error
The command line flag "-v ON_ERROR_STOP=1" should be used when invoking psql to make sure errors are not suppressed.

Example: psql -v ON_ERROR_STOP=1 -q -f awesome-file.sql my-app-db

Fixes #23818.
@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth
Member

kaspth commented May 16, 2016

@rails-bot rails-bot assigned arthurnn and unassigned kaspth May 16, 2016

@arthurnn arthurnn merged commit 267489a into rails:master Jun 17, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@arthurnn

This comment has been minimized.

Show comment
Hide comment
@arthurnn

arthurnn Jun 17, 2016

Member

thanks , and sorry for taking so long to review it.

Member

arthurnn commented Jun 17, 2016

thanks , and sorry for taking so long to review it.

@ralinc ralinc deleted the ralinc:fix-silent-fail-on-psql-command branch Jun 17, 2016

@danielricecodes

This comment has been minimized.

Show comment
Hide comment
@danielricecodes

danielricecodes Oct 14, 2017

@ralinc - I wish this feature had shipped with a way to disable this behavior. I upgraded to Rails 5.1 and this change has broken all of my CI and Heroku automation. The CI server appears to be running rake db:structure:load more than once and this change has rendered this rake task non-idempotent.

I have no easy way to disable this feature b/c an environmental flag has been embedded in this psql command tail for me with no easy way to undo this. I for one, do not desire this flag on my system. At least not right now.

I'm also confused by the OP's original issue and am not 100% sure why this would even be approved and merged into ActiveRecord. A command line flag like this can be added on one's own by just appending it onto their command like this: rake db:structure:load ON_ERROR_STOP=1. Futhermore, psql does not suppress the errors it encounters without this flag. They are clearly visible in the Terminal and the Rails log (on OSX at least). If these are suppressed on other systems, then that must be due to platform differences in psql or how those platforms treat STDIN and STDERR streams - but I don't see how that would merit forcing environmental flags onto everyone using ActiveRecord 5.1.

IMHO, this was a hastily implemented and not thoroughly reviewed feature. Is it possible to roll it back? (I know I can by forking ActiveRecord ;) )

danielricecodes commented Oct 14, 2017

@ralinc - I wish this feature had shipped with a way to disable this behavior. I upgraded to Rails 5.1 and this change has broken all of my CI and Heroku automation. The CI server appears to be running rake db:structure:load more than once and this change has rendered this rake task non-idempotent.

I have no easy way to disable this feature b/c an environmental flag has been embedded in this psql command tail for me with no easy way to undo this. I for one, do not desire this flag on my system. At least not right now.

I'm also confused by the OP's original issue and am not 100% sure why this would even be approved and merged into ActiveRecord. A command line flag like this can be added on one's own by just appending it onto their command like this: rake db:structure:load ON_ERROR_STOP=1. Futhermore, psql does not suppress the errors it encounters without this flag. They are clearly visible in the Terminal and the Rails log (on OSX at least). If these are suppressed on other systems, then that must be due to platform differences in psql or how those platforms treat STDIN and STDERR streams - but I don't see how that would merit forcing environmental flags onto everyone using ActiveRecord 5.1.

IMHO, this was a hastily implemented and not thoroughly reviewed feature. Is it possible to roll it back? (I know I can by forking ActiveRecord ;) )

@kubakrzempek

This comment has been minimized.

Show comment
Hide comment
@kubakrzempek

kubakrzempek Aug 15, 2018

@danielricecodes I don't like the change either but have managed to work around this by creating a custom task that leverages .structure_load_flags

namespace :db do
  namespace :structure do
    desc "Disable exit-on-error behaviour when loading db structure in postgresql"
    task disable_errors: :environment do
      ActiveRecord::Tasks::DatabaseTasks.structure_load_flags = ["-v", "ON_ERROR_STOP=0"]
    end
  end
end

So, for my heroku automation it is now bin/rails db:structure:disable_errors db:structure:load

kubakrzempek commented Aug 15, 2018

@danielricecodes I don't like the change either but have managed to work around this by creating a custom task that leverages .structure_load_flags

namespace :db do
  namespace :structure do
    desc "Disable exit-on-error behaviour when loading db structure in postgresql"
    task disable_errors: :environment do
      ActiveRecord::Tasks::DatabaseTasks.structure_load_flags = ["-v", "ON_ERROR_STOP=0"]
    end
  end
end

So, for my heroku automation it is now bin/rails db:structure:disable_errors db:structure:load

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment