Skip to content
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

Fix a bug with returning_disabled when using the postgresql adapter #21697

Merged

Conversation

gdeglin
Copy link
Contributor

@gdeglin gdeglin commented Sep 21, 2015

The returning_disabled configuration option is required to make postgresql partitioning triggers work. This commit fixes a bug where an invalid query would be made in cases where returning_disabled was true and objects were created with no attributes defined.

This fixes one of the issues I identified in #21659

@rails-bot
Copy link

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @senny (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.

The returning_disabled configuration option is required to make postgresql partitioning triggers work. This commit fixes a bug where an invalid query would be made in cases where returning_disabled was true and objects were created with no attributes defined.
@gdeglin gdeglin force-pushed the fix_returning_disabled_default_values_bug branch from ad47a84 to 871f453 Compare September 21, 2015 06:21
@gdeglin
Copy link
Contributor Author

gdeglin commented Sep 23, 2015

For extra clarity on this bug and fix, here's a very simple failing testcase: https://gist.github.com/gdeglin/a32541807ba366d57c57

The stack trace of the failure is:

# Running:

D, [2015-09-22T23:18:52.424245 #65833] DEBUG -- :    (0.1ms)  BEGIN
D, [2015-09-22T23:18:52.428852 #65833] DEBUG -- :   SQL (0.4ms)  INSERT INTO "users" DEFAULT VALUES
D, [2015-09-22T23:18:52.430462 #65833] DEBUG -- :   SQL (0.2ms)  SELECT currval('_id_seq')
D, [2015-09-22T23:18:52.430666 #65833] DEBUG -- :    (0.1ms)  ROLLBACK
E

Finished in 0.017891s, 55.8932 runs/s, 0.0000 assertions/s.

  1) Error:
BugTest#test_association:
ActiveRecord::StatementInvalid: PG::InFailedSqlTransaction: ERROR:  current transaction is aborted, commands ignored until end of transaction block
: SELECT currval('_id_seq')
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb:611:in `async_exec'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb:611:in `block in exec_no_cache'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:514:in `block in log'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activesupport/lib/active_support/notifications/instrumenter.rb:20:in `instrument'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:508:in `log'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb:611:in `exec_no_cache'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb:603:in `execute_and_clear'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb:160:in `exec_query'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb:728:in `last_insert_id_result'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb:199:in `exec_insert'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:109:in `insert'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb:14:in `insert'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/relation.rb:65:in `insert'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/persistence.rb:541:in `_create_record'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/counter_cache.rb:128:in `_create_record'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/locking/optimistic.rb:75:in `_create_record'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/attribute_methods/dirty.rb:132:in `_create_record'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/callbacks.rb:310:in `block in _create_record'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activesupport/lib/active_support/callbacks.rb:119:in `call'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activesupport/lib/active_support/callbacks.rb:119:in `call'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activesupport/lib/active_support/callbacks.rb:498:in `block (2 levels) in compile'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activesupport/lib/active_support/callbacks.rb:441:in `call'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activesupport/lib/active_support/callbacks.rb:441:in `call'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activesupport/lib/active_support/callbacks.rb:94:in `__run_callbacks__'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activesupport/lib/active_support/callbacks.rb:752:in `_run_create_callbacks'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/callbacks.rb:310:in `_create_record'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/timestamp.rb:64:in `_create_record'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/persistence.rb:521:in `create_or_update'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/callbacks.rb:306:in `block in create_or_update'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activesupport/lib/active_support/callbacks.rb:119:in `call'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activesupport/lib/active_support/callbacks.rb:119:in `call'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activesupport/lib/active_support/callbacks.rb:498:in `block (2 levels) in compile'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activesupport/lib/active_support/callbacks.rb:441:in `call'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activesupport/lib/active_support/callbacks.rb:441:in `call'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activesupport/lib/active_support/callbacks.rb:94:in `__run_callbacks__'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activesupport/lib/active_support/callbacks.rb:752:in `_run_save_callbacks'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/callbacks.rb:306:in `create_or_update'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/suppressor.rb:41:in `create_or_update'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/persistence.rb:125:in `save'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/validations.rb:43:in `save'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/attribute_methods/dirty.rb:21:in `save'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/transactions.rb:297:in `block (2 levels) in save'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/transactions.rb:373:in `block in with_transaction_returning_status'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:212:in `block in transaction'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb:183:in `within_new_transaction'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:212:in `transaction'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/transactions.rb:209:in `transaction'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/transactions.rb:370:in `with_transaction_returning_status'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/transactions.rb:297:in `block in save'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/transactions.rb:312:in `rollback_active_record_state!'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/transactions.rb:296:in `save'
    /Users/George/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/rails-ef7791ca1460/activerecord/lib/active_record/persistence.rb:34:in `create'
    insert_returning_bug.rb:53:in `test_association'

You can see that Rails attempted to query a non-existent sequence. This is because it fails to fetch the table name correctly. This patch corrects a regular expression so that it does correctly fetch the table name in this case, and then queries the correct sequence.

assert_equal expect.to_i, result.rows.first.first
end


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please be more prudent about whitespace issues like this (your end lines had trailing whitespace as well). We have to merge manually from the command line to fix issues like this, which takes a lot more time and means we can process fewer issues each day.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll keep this in mind for next time. Thanks!

@sgrif sgrif merged commit 871f453 into rails:master Sep 24, 2015
sgrif added a commit that referenced this pull request Sep 24, 2015
…_values_bug

Fix a bug with returning_disabled when using the postgresql adapter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants