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.

Sign up for GitHub

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

Jump to bottom

Clarify DatabaseStatements#execute docs re: memory usage. #23188

Merged
merged 1 commit into from Jan 29, 2016
Merged

Clarify DatabaseStatements#execute docs re: memory usage. #23188

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb
View file
Open in desktop
Expand Up @@ -69,7 +69,11 @@ def select_rows(sql, name = nil, binds = [])
end
undef_method :select_rows

# Executes the SQL statement in the context of this connection.
# Executes the SQL statement in the context of this connection and returns
# the raw result from the connection adapter.
# Note: depending on your database connector, the result returned by this
# method may be manually memory managed. Consider using the exec_query
Copy link
Member

Choose a reason for hiding this comment

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

what means manually memory managed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The PG::Result object, for example, contains manually malloc'd memory that the Ruby GC doesn't see. This means that there is no GC pressure caused by this memory usage even though the VM's memory usage has increased.

The only way to free this memory is to call clear on the result, or manually trigger the GC (the result object has a GC hook that calls clear).

Copy link
Member

Choose a reason for hiding this comment

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

Given we are recommending using another method because of its dangerous why keep this method as public API?

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 would prefer that we not, but it'd be a major breaking change (execute is recommend all over the internet), and also execute handles multi-statement SQL strings while exec_query does not. See #22331 for more details.

Copy link
Member

Choose a reason for hiding this comment

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

I see. I'd also remove execute from the public API and agree that would be a breaking change, but is not that a breaking change that would bring benefits?

@matthewd WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we remove execute, I'd like to rename it execute_raw and potentially add some safe variants that handle multiple statement SQL strings (as well as handling SQL binds in the same way as execute rather than requiring the types to be passed like exec_query does. Currently there's no best-of-both-worlds function.

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure about removing this from public API. As far as I can tell the mysql adapter does not have that problem. So changing the parent class doc might not be a good idea too.
This seems like an implementation detail, so I would document this on the PG adapter, and thats it.

# wrapper instead.
def execute(sql, name = nil)
end
undef_method :execute
Expand Down
2 changes: 2 additions & 0 deletions activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb
View file
Open in desktop
Expand Up @@ -128,6 +128,8 @@ def query(sql, name = nil) #:nodoc:

# Executes an SQL statement, returning a PGresult object on success
# or raising a PGError exception otherwise.
# Note: the PGresult object is manually memory managed; if you don't
# need it specifically, you many want consider the exec_query wrapper.
def execute(sql, name = nil)
log(sql, name) do
@connection.async_exec(sql)
Expand Down