So I''ve finally bitten the bullet and patched the AR Oracle adapter to use real OCI bind variables. This has a measurable impact on the client side, and a huge impact on a high volume Oracle server due to the sql hard parses that it saves. The implementation involves some funky regex''s on the sql sent along to the connection adapter, so I''m going to do some more extensive testing within my own app before posting to Trac. A few questions though: 1. I''ll fix any issues I find, but since this is done by post-processing the sql there''s a chance that some funky hand-coded sql could make this fail. So I''m thinking about making this a configurable option -- #enable_oci_binding or somesuch. Thoughts? 2. Do other db''s support a similar concept with a similar benefit? Should this functionality be implemented at a higher level (ie., in the abstract adapter) to make it usable for other dbs? As an aside, only the pretty comprehensive AR unit tests make a patch like this even possible to think about. So kudos to everyone on that point.
> The implementation involves some funky regex''s on the sql sent along to > the connection adapter, so I''m going to do some more extensive testing > within my own app before posting to Trac.I like the idea, however perhaps it''s not ambitious enough? If instead of doing the value interpolation immediately, we could hold off and pass the full sql string down to the adapter. ["FULL SQL STRING", bunch, of, values] I''ve heard from thoughtworks that the performance difference for oracle is appreciable. It''s probably true for other DBs too. The largest benefit though would be allowing people to insert huge blobs without a ... 65k sql statement.> A few questions though: > > 1. I''ll fix any issues I find, but since this is done by post-processing > the sql there''s a chance that some funky hand-coded sql could make this > fail. So I''m thinking about making this a configurable option -- > #enable_oci_binding or somesuch. Thoughts? > > 2. Do other db''s support a similar concept with a similar benefit? > Should this functionality be implemented at a higher level (ie., in the > abstract adapter) to make it usable for other dbs? > > As an aside, only the pretty comprehensive AR unit tests make a patch > like this even possible to think about. So kudos to everyone on that point. > > _______________________________________________ > Rails-core mailing list > Rails-core@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails-core >-- Cheers Koz
On 5/25/06, Michael Koziarski <michael@koziarski.com> wrote:> > The implementation involves some funky regex''s on the sql sent along to > > the connection adapter, so I''m going to do some more extensive testing > > within my own app before posting to Trac. > > I like the idea, however perhaps it''s not ambitious enough? > > If instead of doing the value interpolation immediately, we could hold > off and pass the full sql string down to the adapter. > > ["FULL SQL STRING", bunch, of, values] > > I''ve heard from thoughtworks that the performance difference for > oracle is appreciable. It''s probably true for other DBs too. > > The largest benefit though would be allowing people to insert huge > blobs without a ... 65k sql statement. > >Yes, please oh please. This would let us do all kinds of interesting things in plugins, as well.
Michael A. Schoen wrote:> So I''ve finally bitten the bullet and patched the AR Oracle adapter to > use real OCI bind variables. This has a measurable impact on the client > side, and a huge impact on a high volume Oracle server due to the sql > hard parses that it saves. > > The implementation involves some funky regex''s on the sql sent along to > the connection adapter, so I''m going to do some more extensive testing > within my own app before posting to Trac. > > A few questions though: > > 1. I''ll fix any issues I find, but since this is done by post-processing > the sql there''s a chance that some funky hand-coded sql could make this > fail. So I''m thinking about making this a configurable option -- > #enable_oci_binding or somesuch. Thoughts? > > 2. Do other db''s support a similar concept with a similar benefit? > Should this functionality be implemented at a higher level (ie., in the > abstract adapter) to make it usable for other dbs?I think so. Ideally all the databases that support bind variables would use them, falling back to the current quoted interpolation of values for bindings that do not support them. I would like to see this patch myself and could try it on our apps. So the sooner you open a Trac ticket, the better :)> As an aside, only the pretty comprehensive AR unit tests make a patch > like this even possible to think about. So kudos to everyone on that point.Ditto. Regards, Blair -- Blair Zajac, Ph.D. <blair@orcaware.com> Subversion training, consulting and support http://www.orcaware.com/svn/
On May 25, 2006, at 3:07 PM, Michael A. Schoen wrote:> So I''ve finally bitten the bullet and patched the AR Oracle adapter > to use real OCI bind variables. This has a measurable impact on the > client side, and a huge impact on a high volume Oracle server due > to the sql hard parses that it saves.I assume bind variables and prepared statements are orthogonal.> 2. Do other db''s support a similar concept with a similar benefit? > Should this functionality be implemented at a higher level (ie., in > the abstract adapter) to make it usable for other dbs?Yes: the native postgres bindings support bind variables. In any case, we can move the existing behavior to the abstract adapter as a fallback.> As an aside, only the pretty comprehensive AR unit tests make a > patch like this even possible to think about. So kudos to everyone > on that point.And kudos to you! Thank you for your conscientious maintainership. jeremy
Michael Koziarski wrote:>> The implementation involves some funky regex''s on the sql sent along to >> the connection adapter, so I''m going to do some more extensive testing >> within my own app before posting to Trac. > > > I like the idea, however perhaps it''s not ambitious enough? > > If instead of doing the value interpolation immediately, we could hold > off and pass the full sql string down to the adapter. > > ["FULL SQL STRING", bunch, of, values]Agreed.> I''ve heard from thoughtworks that the performance difference for > oracle is appreciable. It''s probably true for other DBs too. > > The largest benefit though would be allowing people to insert huge > blobs without a ... 65k sql statement.And query caching. To work around lack of bind variables, our Oracle DBA has enabled cursor sharing, which is an extra pass that Oracle does over the SQL. So it''ll take two separate SQL statements, say SELECT * FROM post WHERE id = 123; SELECT * FROM post WHERE id = 456; which would normally require Oracle to completely parse the SQL, look at the rights of the connected user, develop an execution plan and execute it, separately for each query. With the cursor sharing, it converts them into SELECT * FROM post WHERE id = :SYS_B_0; which is then bound to the value. Then Oracle gets to leverage the pre-existing query execution plan. However, having Oracle parse the SQL to determine if the SQL is similar or not is not ideal. Here''s a good writeup of it, which links to a PDF that describes what Oracle does. http://www.rittman.net/archives/2004/06/bjrn_engsig_on_bind_variables.html Blair
Blair Zajac wrote:> To work around lack of bind variables, our Oracle DBA has enabled cursor > sharing, which is an extra pass that Oracle does over the SQL. So it''ll > take two separate SQL statements, sayYep, that''s what we''re doing right now in production as well. The biggest issues are: a) it''s still a chunk of extra work for the already overworked db, and b) with cursor_sharing set to `similar`, any sql statements with non-equality operators don''t share properly -- and setting it to `exact` leads to other issues.
Michael Koziarski wrote:> If instead of doing the value interpolation immediately, we could hold > off and pass the full sql string down to the adapter. > > ["FULL SQL STRING", bunch, of, values]Would be much nicer, but I wasn''t sure how to even approach that, given how frequently the sql is manipulated as a string.
Jeremy Kemper wrote:>> 2. Do other db''s support a similar concept with a similar benefit? >> Should this functionality be implemented at a higher level (ie., in >> the abstract adapter) to make it usable for other dbs? > > Yes: the native postgres bindings support bind variables. In any case, > we can move the existing behavior to the abstract adapter as a fallback.Ok, I''ll plan on moving my code up to abstract tonight and submit to Trac, showing how it could be applied to any adapter that supports it.
On Thu, May 25, 2006 at 04:14:03PM -0700, Jeremy Kemper wrote:> >As an aside, only the pretty comprehensive AR unit tests make a > >patch like this even possible to think about. So kudos to everyone > >on that point. > > And kudos to you! Thank you for your conscientious maintainership.Just wanted to echo Jeremy''s sentiment. Thanks for the work you do on the Oracle adapter. marcel -- Marcel Molina Jr. <marcel@vernix.org>
Michael A. Schoen wrote:> Michael Koziarski wrote: >> If instead of doing the value interpolation immediately, we could hold >> off and pass the full sql string down to the adapter. >> >> ["FULL SQL STRING", bunch, of, values] > > Would be much nicer, but I wasn''t sure how to even approach that, given > how frequently the sql is manipulated as a string.I''ve posted my patch for the regex approach to bind variables: http://dev.rubyonrails.org/ticket/5199 I decided to just leave it in the oracle adapter for now, given that there appears to be interest in doing this the "right" way, as Koz writes above. And though I''ve gotten the regex approach to work well enough (it passes all the AR units tests and my own app works properly), it still feels brittle and silly (esp. since it binds ALL literals in the statement, often many more than really necessary). IOW, I''d rather that this patch NOT be applied -- in fact I think my approach should be looked at with good humor, and then thrown away, in favor of representing an executable sql statement as an array of statement and bind values. With each connection adapter handling as appropriate, interpolating in Ruby if necessary or using native bind variables. That approach will also mean changing how we generate UPDATE and INSERT statements, since they are currently generated from scratch as literal sql, and we''ll want all the values to be bound.
Hi all, On May 26, 2006, at 10:47 AM, Michael A. Schoen wrote:> IOW, I''d rather that this patch NOT be applied -- in fact I think > my approach should be looked at with good humor, and then thrown > away, in favor of representing an executable sql statement as an > array of statement and bind values. With each connection adapter > handling as appropriate, interpolating in Ruby if necessary or > using native bind variables. > > That approach will also mean changing how we generate UPDATE and > INSERT statements, since they are currently generated from scratch > as literal sql, and we''ll want all the values to be bound.For what its worth, this sounds similar to the problem I had when trying to figure out how to adapt ActiveRecord to non-SQL data sources. What intrigued me is that the high-level Rails APIs actually had the right granularity, but since everything got munged into a single SQL string it all had to be re-parsed out again. :-( My approach was to define a new Query object, return by the connection, that preserved that abstraction. My "ActiveData" subclass then overrode the sql-manipulation methods to use that instead, e.g.: def update_all(updates, conditions = nil) sql = connection.new_query() sql.add("UPDATE", table_name) sql.add("SET", sanitize_sql(updates)) add_conditions!(sql, conditions) sql.execute(:update, "#{name} Update") end Of course, this would fail with user-generated SQL, but otherwise it seemed like it should handle everything correctly. http://www.opendarwin.org/~drernie/C499496031/E20060222133959/index.html Haven''t done much since, and not sure how relevant it is, but it seems like a similar enough problem to be worth mentioning. I''d certainly be interested in whatever the "right" solution to Michael''s problem is, to see if that helps me with mine. -- Ernie P.
Michael A. Schoen wrote:> IOW, I''d rather that this patch NOT be applied -- in fact I think my > approach should be looked at with good humor, and then thrown away, in > favor of representing an executable sql statement as an array of > statement and bind values. With each connection adapter handling as > appropriate, interpolating in Ruby if necessary or using native bind > variables. > > That approach will also mean changing how we generate UPDATE and INSERT > statements, since they are currently generated from scratch as literal > sql, and we''ll want all the values to be bound.So...all aboard? I''m game to attempt a real implementation, so long as I''ve got core approval for a fairly substantial change to how sql''s passed around internally. Also, could I get a 1-line code sample of how postgres bind variables would be used?
And let''s not forget the most important reason to do this: this is on the path to making Rails TRULY enterprisy! :-) We''re actually getting serious kickback from using Rails at one of our clients because of this very problem. We''ve hacked around the issue for now but client is still not happy. We''re flooding the database. Also, I would love to at least see the capability of passing bind variables directly to adapters from iBatis. I reckon we could easily make it backwards compatible, something akin to: if connection.supports_bind_variables? connection.select([''SELECT *'', a, b, c]) else connection.select(quote([''SELECT *'', a, b, c])) end Or, by simply wrapping non-bind-supporting adapters with a binding decorator. Or, just fix all the darned adapters, that''s why we have a test suite after all: so we can be BRAVE. :-) Cheers, Jon On 5/26/06, Michael Koziarski <michael@koziarski.com> wrote:> I like the idea, however perhaps it''s not ambitious enough? > > If instead of doing the value interpolation immediately, we could hold > off and pass the full sql string down to the adapter. > > ["FULL SQL STRING", bunch, of, values] > > I''ve heard from thoughtworks that the performance difference for > oracle is appreciable. It''s probably true for other DBs too. > > The largest benefit though would be allowing people to insert huge > blobs without a ... 65k sql statement. > > > > > A few questions though: > > > > 1. I''ll fix any issues I find, but since this is done by post-processing > > the sql there''s a chance that some funky hand-coded sql could make this > > fail. So I''m thinking about making this a configurable option -- > > #enable_oci_binding or somesuch. Thoughts? > > > > 2. Do other db''s support a similar concept with a similar benefit? > > Should this functionality be implemented at a higher level (ie., in the > > abstract adapter) to make it usable for other dbs? > > > > As an aside, only the pretty comprehensive AR unit tests make a patch > > like this even possible to think about. So kudos to everyone on that point. > > > > _______________________________________________ > > Rails-core mailing list > > Rails-core@lists.rubyonrails.org > > http://lists.rubyonrails.org/mailman/listinfo/rails-core > > > > > -- > Cheers > > Koz > _______________________________________________ > Rails-core mailing list > Rails-core@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails-core >
Michael A. Schoen wrote:> Michael A. Schoen wrote: >> IOW, I''d rather that this patch NOT be applied -- in fact I think my >> approach should be looked at with good humor, and then thrown away, in >> favor of representing an executable sql statement as an array of >> statement and bind values. With each connection adapter handling as >> appropriate, interpolating in Ruby if necessary or using native bind >> variables. >> >> That approach will also mean changing how we generate UPDATE and >> INSERT statements, since they are currently generated from scratch as >> literal sql, and we''ll want all the values to be bound. > > So...all aboard? I''m game to attempt a real implementation, so long as > I''ve got core approval for a fairly substantial change to how sql''s > passed around internally. > > Also, could I get a 1-line code sample of how postgres bind variables > would be used?Still looking for a few nods of approval from the core team before I dig into this.
> Still looking for a few nods of approval from the core team before I dig > into this.I''m happy to see work go into this. Real bind variables would be nice indeed. Applying the patch will, as ever, depend on the specific implementation, of course. But it''s a worthy pursuit! -- David Heinemeier Hansson http://www.loudthinking.com -- Broadcasting Brain http://www.basecamphq.com -- Online project management http://www.backpackit.com -- Personal information manager http://www.rubyonrails.com -- Web-application framework