Clarify DatabaseStatements#execute docs re: memory usage.

[jcoleman]

@eileencodes This is just a docs clarification (but on the method docs, not the rails guides.)

[rails-bot]

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

[rafaelfranca]

what means manually memory managed?

[jcoleman]

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).

[rafaelfranca]

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

[jcoleman]

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.

[rafaelfranca]

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?

[jcoleman]

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.

[arthurnn]

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.

[matthewd]

I still don't understand how one would read the contents of the intended multiple queries, without knowing you had a PG::Result. And if you know that, it's not our responsibility to tell you how it works -- it's not our class.

execute is the intended, widely-published API for running non-query database statements. For that purpose, it works perfectly.

If you want to do something more complicated, you should probably just use raw_connection (which again, we're not going to document, because it's not ours).

[jcoleman]

@matthewd I don't intend to read the contents of multiple queries; typically multiple queries is either multiple modifying statements (update/delete etc.) or one or more modifying statements followed by a single select.

Regardless, even if you don't have multiple statements, exec_query and friends still leave a lot to be desired, because not everyone wants prepared statements generated automatically, and, more importantly, the interface to those methods is significantly more difficult to use since it doesn't accepts bind params in the same manner as execute .