Repost: Why is rails generating bad SQL?

Rails
1

It looks like I am missing something obvious. ActiveRecord is
generating really bad SQL for this configuration, and I can’t quite
figure it out.

I’ve instrumented ActiveRecord enough to localize the problem somewhat,
and generally by this time I would have a good idea of what I was
missing because it’s all in the source.

It appears that something in ActiveRecord.construct_finder_sql is
choking on something I am doing. Any ideas would be appreciated.

Thanks in advance,
David J.

Here’s my code that is failing:

def create

changed because we are instantiating the presentations here

may not be needed if we instantiate presentations in the initialize,

but that will require thinking because we already have a use for

the initialize method.

@question = Question.new

STDOUT.print("\r\n------------------------------\r\n")
STDOUT.print(“Looking for Parent= “,params[:parent_quiz],”\r\n”)
aParent = Quiz.find(:conditions => [“id=?”,params
[:parent_quiz]], :limit => 1)
STDOUT.print(“Found Parent= “,aParent,”\r\n”) # aParent is returning
nil, but it shouldbe a valid object

Here’s the STDOUT log (I have instrumented ActiveRecord to print the SQL
before it tries to run it):

Looking for Parent= ff2d7022-be0a-11da-9f01-00400506faf5

Initial request = nil

SQL Conditions = ‘nil’

Have one ID to check: ff2d7022-be0a-11da-9f01-00400506faf5

find all - finder_sql= SELECT FIRST 1 * FROM quizzes WHERE (quizzes.id
= '— !map:HashWithIndifferentAccess
ff2d7022-be0a-11da-9f01-00400506faf5: “”
')

I instrumented ActiveRecord.find

2

More data:

The faulty option appears to be generated in this code snippet from
ActiveRecord.first

              raise RecordNotFound, "Couldn't find #{name} with ID=#{ids.first}#{conditions}"
            end
          else
            # Find multiple ids
            ids_list = ids.map { |id| sanitize(id) }.join(',')
            result   = find(:all, options.merge({ :conditions => "#{table_name}.#{primary_key} IN (#{ids_list})#{conditions}"}))
            if result.size == ids.size
              return result
            else
              raise RecordNotFound, "Couldn't find all #{name.pluralize} with IDs (#{ids_list})#{conditions}"
            end
        end
3

I got past the initial problem (but I had to dig bloody deep to find
it). Apparently, params[:parent_quiz] was returning a
HashWithIndifferentAccess. Casting it as a String using params
[:parent_quiz].to_s resolved that problem.

However, I am still getting bad SQL in the find invoked by the save.
The parent_id should be quoted.

As I am still a ruby-newbie, I would greatly appreciate input from more
experienced Ruby folks as to the ideal resolution. I believe that it is
to replace configuration[:scope].to_s with quote(configuration[:scope]).

Ideas anyone?

Example:

SELECT FIRST 1 * FROM questions WHERE (parent_id = ff2d7022-
be0a-11da-9f01-00400506faf5) ORDER BY seq DESC

The code snippet from ActiveRecord.Acts.List is generating the SQL with
unquoted text:

      if configuration[:scope].is_a?(Symbol)
        scope_condition_method = %(
          def scope_condition
            if #{configuration[:scope].to_s}.nil?
              "#{configuration[:scope].to_s} IS NULL"
            else

#Unquoted SQL is generated by this line.
“#{configuration[:scope].to_s} = #{#{configuration
[:scope].to_s}}”
end
end
)
else
scope_condition_method = “def scope_condition() “#
{configuration[:scope]}” end”
end

4

I have submitted a patch for the failure to quote the ID. I would
appreciate feedback for any corrections that may be necessary. I still
don’t have the hang of Ruby’s style of inline string substitutions.

Ticket URL: http://dev.rubyonrails.org/ticket/4538

Thanks

5

On Sat, 2006-04-01 at 22:00 -0800, Tom M. wrote:

This line should read:

aParent = Quiz.find(params[:parent_quiz])

Still fails … needs to be Quiz.find(params[:parent_quiz].to_s). For
some reason params[:parent_quiz] is a HashWithIndifferentAccess (not a
string) that needs to be dereferenced before it is passed down into the
lower layers.

Also, see my patch to ActiveRecord.Acts.List. Primary keys that are not
integers are not properly quoted on the reselect that follows the
insert.

6

On Sat, 2006-04-01 at 22:00 -0800, Tom M. wrote:

Not clear why you’d name the id of the parent record :parent_quiz,
seems
:parent_id would be much more readable.

Because (1) there are places in the overall data model where the
“parent” relationship is a bit ambiguous, (2) I am trying to focus on
the learning Ruby and Rails, (3) this will be refactored into session
properties once I get the relational model idioms sorted out (I hate the
idea of sending session information back to the browser).

This particular application is an exercise I do whenever I start
learning a new framework. It happens to be immediately useful to me, but
there is no requirement that I succeed in completing the task. I don’t
expect to have to maintain this code base after it is complete.

It is just complex enough and breaks enough framework builders’
expectations to expose weaknesses in the handling of relational models.
But it is not so large or complex as to become unmanageable whatever the
problems are that I run into.

Thanks for the feedback. I would really appreciate your review of my
proposed patch at http://dev.rubyonrails.org/ticket/4538. Thanks.

7

On Apr 1, 2006, at 11:57 AM, David J. wrote:

It looks like I am missing something obvious. ActiveRecord is
generating really bad SQL for this configuration, and I can’t quite
figure it out.

snip…

STDOUT.print(“Looking for Parent= “,params[:parent_quiz],”\r\n”)
aParent = Quiz.find(:conditions => [“id=?”,params
[:parent_quiz]], :limit => 1)

This line should read:

aParent = Quiz.find(params[:parent_quiz])

Not clear why you’d name the id of the parent record :parent_quiz, seems
:parent_id would be much more readable.

–
– Tom M.

8

On Apr 2, 2006, at 10:41 AM, David J. wrote:

Here’s my code that is failing:
STDOUT.print(“Looking for Parent= “,params[:parent_quiz],”\r\n”)
the
lower layers.

Two things:

  1. If you’re getting a HashWithIndifferentAccess, then something is
    wrong.
    Perhaps (just guessing) you need something like:

    params[:parent_quiz][:id]

  2. Id’s should be integers, not strings.

Also, see my patch to ActiveRecord.Acts.List. Primary keys that
are not
integers are not properly quoted on the reselect that follows the
insert.

Rails doesn’t support primary keys that aren’t integers.

It’s likely more than acts_as_list will need to be patched to “fix”
this.

–
– Tom M.

9

On Apr 2, 2006, at 2:38 PM, David J. wrote:

recipe for long-term problems (been there done that).
Sorry, I didn’t make this clear enough. I thought (see below) that I was
stating fact, not giving advice. Rails assumes that IDs are integers.

Also, see my patch to ActiveRecord.Acts.List. Primary keys that
are not integers are not properly quoted on the reselect that
follows the insert.

Rails doesn’t support primary keys that aren’t integers.
According to “Agile” book, Rails does support this. According to
reality, it appears to be just a very thin sliver shy of full support.

You’re right! It’s been a while since I read it, and I guess I’ve heard
so many people recommend against it, I forgot it was true. That said,
it seems it doesn’t work properly, so perhaps I was “correct” in a
rather
backward way…

–
– Tom M.

10

On Sun, 2006-04-02 at 11:47 -0700, Tom M. wrote:

snip…

@question = Question.new

some reason params[:parent_quiz] is a HashWithIndifferentAccess (not a
string) that needs to be dereferenced before it is passed down into
the
lower layers.

Two things:

  1. If you’re getting a HashWithIndifferentAccess, then something is
    wrong.
    I agree. But what?

params[:parent_quiz] is (allegedly) coming directly from the browser
parameters at this point. The only thing that processes it before I get
to it is the Rails framework. If there is a bug in that part of the
framework then I need to understand a bit more before I expend effort
trying to patch it.

Perhaps (just guessing) you need something like:

params[:parent_quiz][:id]

Tried it. Got the Rails equivalent of “Watchu talking 'bout
Willis?” (see stdout log)

Couldn’t find Quiz without an ID

RAILS_ROOT: script/…/config/…

Application Trace | Framework Trace | Full Trace
/usr/lib/ruby/gems/1.8/gems/activerecord-1.13.2/lib/active_record/base.rb:409:in
find' ./script/../config/../app/controllers/question_controller.rb:39:increate’

  1. Id’s should be integers, not strings.

The assumption behind this is that all databases are centralized. There
are many applications where databases must be decentralized and merged
on demand. Integers do not guarantee uniqueness once you have split the
database across multiple pieces of hardware that cannot communicate with
each other. Assigning integer ranges to hardware systems is a sure
recipe for long-term problems (been there done that).

An example of this is the first database application I wrote for a
construction company. One of the infrastructure constraints was that
most sites went in before there was communications. The cell tower was
usually erected about 2 weeks after crews demobilized. But we had to be
able to merge the field and office data on demand at least once per week
during construction.

The use of UUID’s as primary keys is accepted practice for this sort of
application. Since most DBMS’ do not support UUID’s natively yet,
representing them as strings is a normal practice. UUID’s are
guaranteed unique in the world until the year 3040 or so because they
combine physical characteristics of the generating machine, time stamp,
and a random factor to generate the resulting value.

Also, see my patch to ActiveRecord.Acts.List. Primary keys that
are not
integers are not properly quoted on the reselect that follows the
insert.

Rails doesn’t support primary keys that aren’t integers.
According to “Agile” book, Rails does support this. According to
reality, it appears to be just a very thin sliver shy of full support.

It’s likely more than acts_as_list will need to be patched to “fix”
this.

I suspect that the other acts_as… methods would also require the same
patch.

I had to adapt a bit, but review of the Rails code showed me that if the
class provides an ID on creation, Rails will not override it. I have a
UUID helper mixin that I lifted from a Rails Blog that is working just
fine on row creation.

A large part of my purpose in using this particular model is to see how
far I can take Rails’ constructs. If it can’t handle my needs then I
will simply move on. So far, the only issues I have run into are in the
class of minor implementation tweaks (maturity) rather than baseline
design issues.

11

On Mon, 2006-04-03 at 13:45 +0200, Jakob S. wrote:

How does your params hash actually look? If you check your logfile, the
it should be visible at the beginning of each request. Does it actually
contain the values you expect?

Yes, it contains the correct values. I’ll revisit this later this week.
I have to prepare for an interview, prepare for a major installation on
a live system this weekend, and learn to use svn diff to submit my
proposed patch.

and you use a mixin to handle the uuid?
It complicates the schema and makes merging the distributed databases
just a tad less straightforward. With the UUID as the PK, I can merge
databases without writing any code. The integer introduces the need to
actually write a program (or generator, or trigger).

You can then do stuff like Quiz.find_by_uuid(params[:parent_quiz])
(assuming params[:parent_quiz] actually holds your uuid).

That said, I imagine it would be nice if non-integer id’s worked just
dandy out of the box. Not something I’ve had a need for myself, though.

It’s not the most common paradigm. But, it’s a useful one for real
distributed applications and for testing the limits of new frameworks.

12

On Sun, 2006-04-02 at 22:45 -0700, Tom M. wrote:

Rails doesn’t support primary keys that aren’t integers.
According to “Agile” book, Rails does support this. According to
reality, it appears to be just a very thin sliver shy of full support.

You’re right! It’s been a while since I read it, and I guess I’ve heard
so many people recommend against it, I forgot it was true. That said,
it seems it doesn’t work properly, so perhaps I was “correct” in a
rather
backward way…

First tenet of debate - a fact rebuts all logic. You win (until we get
a couple of patches out) :o)

Please accept my apologies for the tone of my response.

The recommendation against strings is because many poorly designed data
models use values that are actually meaningful to the business as their
primary keys. The argument is not against strings, per se, but against
uncontrolled free form data that is meaningful to the business and may
be subject to change.

Integer sequences (tokens) are the easiest mechanism to implement that
separates the primary key from business meaning. Since the majority of
RDBMS applications are centralized, or at least localized in nature, it
is the most common mechanism used.

There are other mechanisms that guarantee uniqueness, and some of those
must be considered when you start using DRDBMS and you have to merge
data across platforms that have only spurious communications. A string
is a “least common denominator” that can represent the results of any
suitable keying algorithm (including integers).

Have you had a chance to review my proposed patch? I am still not
entirely comfortable with the inline string substitution for self
modifying code, so I am not convinced that my solution won’t break
someone else’ project.

13

David J. wrote:

Perhaps (just guessing) you need something like:

params[:parent_quiz][:id]

Tried it. Got the Rails equivalent of “Watchu talking 'bout
Willis?” (see stdout log)

Couldn’t find Quiz without an ID

How does your params hash actually look? If you check your logfile, the
it should be visible at the beginning of each request. Does it actually
contain the values you expect?

  1. Id’s should be integers, not strings.

I had to adapt a bit, but review of the Rails code showed me that if the
class provides an ID on creation, Rails will not override it. I have a
UUID helper mixin that I lifted from a Rails Blog that is working just
fine on row creation.

Is anything stopping you from having both an id and a uuid column? Leave
Rails to worry about the id column, which is an integer like it expects,
and you use a mixin to handle the uuid?

You can then do stuff like Quiz.find_by_uuid(params[:parent_quiz])
(assuming params[:parent_quiz] actually holds your uuid).

That said, I imagine it would be nice if non-integer id’s worked just
dandy out of the box. Not something I’ve had a need for myself, though.