I noticed that in the association_preload there are a number of FIXMEs that all boil down to the fact that I ignored table/column name quoting when i first wrote it up.
Ideally to test this I'd need a bunch of tables with interesting table & column names. What's the policy on adding to the tables/models using in tests? Just do it ? Also I noticed there's a whole bunch of files in schema for different databases: does one need to add the declaration for such tables to all of these files? I don't have access to all of those dbs so it would be hard for me to be sure I hadn't messed things up.
Ideally to test this I'd need a bunch of tables with interesting table
& column names. What's the policy on adding to the tables/models using
in tests? Just do it ? Also I noticed there's a whole bunch of files
in schema for different databases: does one need to add the
declaration for such tables to all of these files? I don't have access
to all of those dbs so it would be hard for me to be sure I hadn't
messed things up.
It would be *really* nice if we could tidy up all those active record
tests to use schema.rb, thereby removing most of the hoops you have to
jump through to add a table to the test schema. Unfortunately this is
probably a reasonably large undertaking, so if your time is limited
you're probably best off just getting it working on your own
databases, making an educated guess for the others, and asking people
for help testing.
I personally have an EC2 image I use to test rails on all the other
open source databases.
I'm sure I'm showing my ignorance here, but I don't quite get what needs to be done.
Part of schema.rb is bracketed with if adapter == 'MYSQL', with a comment about things being transitioned from fixtures/db_definitions (which doesn't seem to exist). Those tables that are in the "if mysql" bit are not in sqlite.sql, postgresql.sql etc..., but those bits that aren't are not in the database specific .sql files, so the other databases are obviously using schema.rb partially.
Why is it not a case of just removing the "if mysql" bits ? I'm happy to spend some time looking at this but I don't think I get what's to be done.
Why is it not a case of just removing the "if mysql" bits ? I'm happy
to spend some time looking at this but I don't think I get what's to
be done.
That's about all there is to it, but after you remove it you have to
run the tests, and catch any differences (implicit or otherwise)
between the schema.rb definition and the schema.sql.
That sounds very useful. Is that an AMI you can make public?
I'll have to rebuild it to use the new git repository and make sure I
don't have my AWS keys in the log files somewhere, but it's certainly
something I'm considering doing. Unfortunately I'm not sure when I'll
get the requisite free time. If someone feels hugely motivated and
grabs me on IRC I'm happy to share the tricks / tips that I learned
along the way
I've had a bash at this, and I've got things in a state where things pass in mysql (obviously) and sqlite3 and postgresql. I haven't the faintest clue what will happen with other databases, mostly because I don't have them installed and know even less about those than the rest.
There are some issues I've had along the way though. The schemas aren't quite identical to what they were before, for example the sql files for postgres defined a lot of strings has having length 50 (not for any particular reason as far as I can tell) whereas schema.rb defines them as having length 255. Another one was People.first_name is not null in schema.rb but allows null in postgres' sql file, which caused a failing test (In that case i fixed up the test).
The main thing that is a bit fiddly is how to deal with database differences. So for example postgres has a bunch of tables that are used to test some postgres specific stuff. Right now at the bottom of schema.rb there's a
case adapter_name
when 'Postgres'
do some postgres stuff
...
end
and there's also one at the top of the file which can be used for adapter specific setup. I haven't decided yet if maybe some of that would be better factored out and put somewhere else (where ?)
Anyway, what i've got so far is over at http://github.com/fcheung/rails/tree/master, thoughts appreciated (if the git stuff is messed up it's probably because I don't have the faintest foggiest clue what I'm doing - I've not used git before, but loving it so far!)
I've put the current version of this patch on trac: http://dev.rubyonrails.org/ticket/11598
Right now mysql, postgres and sqlite3 are happy. The databases I don't have access to/am clueless about are probably unhappy.
Which databases are these? I mean, which databases is Rails actively
making an effort to keep the tests green against, and which ones are
known to not work anyway (or can accept breakages)?
Well the ones for which the adapters come with rails (mysql, postgres, sqlite3, sqlite (does anyone actually use sqlite non-3? there's currently failing tests there)) should certainly all be kept happy. I personally don't have the faintest clue how heavily used/important the others are.
Well the ones for which the adapters come with rails (mysql, postgres,
sqlite3, sqlite (does anyone actually use sqlite non-3? there's
currently failing tests there)) should certainly all be kept happy. I
personally don't have the faintest clue how heavily used/important the
others are.
The other databases should be kept green by the maintainers of their
adapters. So sqlite, mysql and postgresql are the ones we're looking
after. The sqlite2 test failures are strange, is there anyone out
there using it at present? Would be good to fix this before 2.1 or at
least document the missing functionality.
In 2.0.2 quote_table_name was a no-op for the sqlite adapter (if not overriden, which sqlite doesn't, it used to just return its argument, but now by default it's just the same as quote_column_name). In particular this messes with some of the eager loading stuff. for example on sqlite 3, running
SELECT DISTINCT "developers".id FROM "developers" LEFT OUTER JOIN "developers_projects" ON "developers_projects".developer_id = "developers".id LEFT OUTER JOIN "projects" ON "projects".id = "developers_projects".project_id WHERE (projects.id = 2) LIMIT 1
returns the selected column as id whereas with sqlite2 it returns it as "developers".id which leads to much confusion In the sqlite adapter there's a key.sub(/^\w+\./, ''), but of course that doesn't catch it now that the key is "developers.id" instead of developers.id. changing the regex to /^"?\w+"?\./ fixes most of the failures, leaving me with
1) Error:
test_should_count_manual_select_with_include(CalculationsTest):
ActiveRecord::StatementInvalid: SQLite::Exceptions::SQLException: near "DISTINCT": syntax error: SELECT COUNT(*) AS count_distinct_accounts_id FROM (SELECT DISTINCT DISTINCT accounts.id FROM "accounts" LEFT OUTER JOIN "companies" ON "companies".id = "accounts".firm_id AND "companies"."type" = 'Firm' )
./test/cases/../../lib/active_record/connection_adapters/abstract_adapter.rb:151:in `log'
./test/cases/../../lib/active_record/connection_adapters/sqlite_adapter.rb:132:in `execute_without_counting'
./test/cases/../../lib/active_record/connection_adapters/sqlite_adapter.rb:353:in `catch_schema_changes'
./test/cases/../../lib/active_record/connection_adapters/sqlite_adapter.rb:132:in `execute_without_counting'
./test/cases/helper.rb:38:in `execute'
./test/cases/../../lib/active_record/connection_adapters/sqlite_adapter.rb:256:in `select'
./test/cases/../../lib/active_record/connection_adapters/abstract/database_statements.rb:7:in `select_all_without_query_cache'
./test/cases/../../lib/active_record/connection_adapters/abstract/query_cache.rb:55:in `select_all'
./test/cases/../../lib/active_record/connection_adapters/abstract/database_statements.rb:13:in `select_one'
./test/cases/../../lib/active_record/connection_adapters/abstract/database_statements.rb:19:in `select_value'
./test/cases/../../lib/active_record/calculations.rb:213:in `execute_simple_calculation'
./test/cases/../../lib/active_record/calculations.rb:124:in `calculate'
./test/cases/../../lib/active_record/calculations.rb:120:in `calculate'
./test/cases/../../lib/active_record/calculations.rb:46:in `count'
./test/cases/calculations_test.rb:249:in `test_should_count_manual_select_with_include'
./test/cases/../../lib/../../activesupport/lib/active_support/testing/setup_and_teardown.rb:59:in `run'
of those only the first one looks like http://dev.rubyonrails.org/ticket/11502: the fix applied also needs to be applied to the sqlite workaround. I'll bung all this in a patch (patches ?) if people are interested.
So, that’s pretty interesting, as in my version of sqlite3 (3.5.7) running the above sql was returning “developers”.id not just id. And thus I had about 9 eager loading test failures, but applying your regex fix sorts them all out. If you were to create a patch with just that regex fix, I’d totally +1 it.
Although, it does make me wonder if there some change between different versions of sqlite3 that cause my version and your version to react differently? Do we need to say something like this sqlite3 adapter works for this version of sqlite3?
I've got an older version (3.4.0). Updating sqlite3 caused me to get those test failures on trunk too.
This does sounds like a bit of a nightmare waiting to bite people on the butt. I'll write up a patch as soon as i have time
So, since you applied the patches I write for those issues sqlite3, mysql & postgres are all happy, sqlite2 has a single failure that I'm looking at. So far, I've left the schema files for the other databases there (along with the code in the aaa_create_tables_test) to load these (but turned off) on the ground that I wouldn't want to make it difficult for someone fix any failures this causes in other databases. Alternatively I could just blow it all away (after all that is what source control is for). Opinions ?
So, since you applied the patches I write for those issues sqlite3,
mysql & postgres are all happy, sqlite2 has a single failure that I'm
looking at. So far, I've left the schema files for the other databases
there (along with the code in the aaa_create_tables_test) to load
these (but turned off) on the ground that I wouldn't want to make it
difficult for someone fix any failures this causes in other databases.
Alternatively I could just blow it all away (after all that is what
source control is for). Opinions ?
I think it's all good to clear those out entirely, so just upload a
patch (using git format-patch on your branch) to wrap this up, and
I'll apply it.