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

Add database configuration to disable advisory locks. #33691

Merged

Conversation

@tgxworld
Copy link
Contributor

@tgxworld tgxworld commented Aug 22, 2018

@rails-bot
Copy link

@rails-bot rails-bot commented Aug 22, 2018

r? @kamipo

(@rails-bot has picked a reviewer for you, use r? to override)

@tgxworld
Copy link
Contributor Author

@tgxworld tgxworld commented Aug 22, 2018

@rails-bot rails-bot assigned matthewd and unassigned kamipo Aug 22, 2018
guides/source/configuring.md Outdated
adapter: postgresql
advisory_locks: false
```

If enabled, Active Record will create up to `1000` prepared statements per database connection by default. To modify this behavior you can set `statement_limit` to a different value:

This comment has been minimized.

@matthewd

matthewd Aug 22, 2018
Member

This is a continuation of the preceding discussion of prepared_statements.

More generally, the above feels a bit heavy-weight for what should be relatively obscure configuration, to me. How about we combine the two: "By default Active Record uses database features like prepared statements and advisory locks. You might need to disable those features if you're using an external connection pooler like PgBouncer: [yaml]"?

activerecord/lib/active_record/connection_adapters/abstract_adapter.rb Outdated
@@ -119,6 +126,10 @@ def initialize(connection, logger = nil, config = {}) # :nodoc:
else
@prepared_statements = false
end

@advisory_locks = self.class.type_cast_config_to_boolean(

This comment has been minimized.

@matthewd

matthewd Aug 22, 2018
Member

I know it's a departure from @prepared_statements, but I suspect this would be clearer as @advisory_locks_enabled, particularly for the callers below.

activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb Outdated
@@ -111,6 +111,7 @@ def supports_virtual_columns?
end

def supports_advisory_locks?
return false unless @advisory_locks

This comment has been minimized.

@matthewd

matthewd Aug 22, 2018
Member

Just returning @advisory_locks[_enabled] seems clear enough here

activerecord/CHANGELOG.md Outdated

```
production:
adpater: postgresql

This comment has been minimized.

@matthewd

matthewd Aug 22, 2018
Member

Typo, and example goes above your name.

@tgxworld tgxworld force-pushed the tgxworld:add_config_to_disable_advisory_locks branch to 20bb397 Aug 22, 2018
@tgxworld
Copy link
Contributor Author

@tgxworld tgxworld commented Aug 22, 2018

@matthewd Thank you for reviewing, I've updated the PR as per your comments.

@matthewd matthewd merged commit 24f6bf0 into rails:master Aug 22, 2018
2 checks passed
2 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@matthewd
Copy link
Member

@matthewd matthewd commented Aug 22, 2018

🤘🏻❤️

:logger,
:prepared_statements,
:lock,
:advisory_locks

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Aug 22, 2018
Member

Should this be :advisory_locks_enabled?

This comment has been minimized.

@rafaelfranca

rafaelfranca Aug 22, 2018
Member

I think it is matching prepared_statements. I'm fine with either one.

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Aug 22, 2018
Member

The instance variable is named @advisory_locks_enabled.

This comment has been minimized.

@tgxworld

tgxworld Aug 22, 2018
Author Contributor

@georgeclaghorn Oops good catch. Thank you for fixing @kamipo 👍

@tgxworld tgxworld deleted the tgxworld:add_config_to_disable_advisory_locks branch Aug 22, 2018
@faucct
Copy link
Contributor

@faucct faucct commented Aug 30, 2018

Could this get a version bump (5.2.2 or 5.2.1.1)? Rails repository is very huge to fetch.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Aug 31, 2018

This is only going to be present in Rails 6.0. And no, we only release when we think it is ready, we can't release after every merged PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.