The build failed.
CHANGES
Further investigation has revealed that this failure is a known issue, and the original patch author has asked for it to be reverted because they can't get it to work on PgSQL. See http://dev.rubyonrails.org/ticket/10480 for details. I can't work out how to make PgSQL play nicely either; I'm in favour of reversion, too.
- Matt
Reversion wouldn’t help. It’s not the count that fails, it’s the SELECT that was added as part of the unit tests for count. The real issue here is that you can’t use has_many :through with :group on Postgres because Postgres is not so forgiving on queries that don’t make sense in theory. The query currently generated is all wrong, but will work on MySQL and perhaps SQLite3.
> Name: test_group_has_many_through_should_use_group_for_count(AssociationsJoinModelTest) > Type: Error > Message: ActiveRecord::StatementInvalid: PGError: ERROR: column "comments.id" must appear in the GROUP BY clause or be used in an aggregate function > > : SELECT "comments".* FROM "comments" INNER JOIN posts ON comments.post_id = posts.id WHERE (("posts".author_id = 1)) GROUP BY comments.post_id > ./test/cases/../../lib/active_record/connection_adapters/abstract_adapter.rb:151:in `log' > ./test/cases/../../lib/active_record/connection_adapters/postgresql_adapter.rb:407:in `execute_without_counting' > ./test/cases/helper.rb:38:in `execute' > ./test/cases/../../lib/active_record/connection_adapters/postgresql_adapter.rb:787:in `select_raw' > ./test/cases/../../lib/active_record/connection_adapters/postgresql_adapter.rb:774: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/base.rb:531:in `find_by_sql' > ./test/cases/../../lib/active_record/base.rb:1245:in `find_every' > ./test/cases/../../lib/active_record/base.rb:502:in `find' > ./test/cases/../../lib/active_record/associations/has_many_through_association.rb:149:in `find_target' > ./test/cases/../../lib/active_record/associations/association_proxy.rb:132:in `load_target' > ./test/cases/../../lib/active_record/associations/association_proxy.rb:123:in `method_missing' > ./test/cases/../../lib/active_record/associations/has_many_through_association.rb:142:in `method_missing' > ./test/cases/associations/join_model_test.rb:607:in `test_group_has_many_through_should_use_group_for_count' > ./test/cases/../../lib/../../activesupport/lib/active_support/testing/setup_and_teardown.rb:59:in `__send__' > ./test/cases/../../lib/../../activesupport/lib/active_support/testing/setup_and_teardown.rb:59:in `run'
Further investigation has revealed that this failure is a known issue, and the original patch author has asked for it to be reverted because they can't get it to work on PgSQL. See http://dev.rubyonrails.org/ticket/10480 for details. I can't work out how to make PgSQL play nicely either; I'm in favour of reversion, too.
Not sure but I think this is the closest PostgreSQL query that should give the expected effect although without an ORDER BY the results are unpredictable.
SELECT DISTINCT ON ( comments.post_id ) "comments".* FROM "comments" INNER JOIN posts ON comments.post_id = posts.id WHERE (("posts".author_id = 1));
So instead of "GROUP BY comments.post_id" you should use "DISTINCT ON ( comments.post_id )". But I have no idea how easy it would be to make activerecord generate this query when using postgresql.
> Further investigation has revealed that this failure is a known issue, and > the original patch author has asked for it to be reverted because they > can't > get it to work on PgSQL. See http://dev.rubyonrails.org/ticket/10480 for > details. I can't work out how to make PgSQL play nicely either; I'm in > favour of reversion, too.
Reversion wouldn't help.
Reversion doesn't help the framework, I agree -- you still can't do what you want to do with the patch reverted. However, you can't do it with PgSQL even with the current patch applied, and the test suite is failing (which irritates people and may even mask other introduced problems because everyone ignores the failure e-mails if they're constant).
It's not the count that fails, it's the SELECT that was added as part of the unit tests for count.
Actually, it's not the query for count (as you say), and it's not something that was introduced specifically for the tests. It's the query that attempts to retrieve the full records on a has_many :through with :group.
The real issue here is that you can't use has_many :through with :group on Postgres because Postgres is not so forgiving on queries that don't make sense in theory. The query currently generated is all wrong, but will work on MySQL and perhaps SQLite3.
I'm pretty sure the query works fine on SQLite3 (although I only tested deeply with MySQL) because the test suite passes fine on SQLite3. It's only PgSQL that has the problem with the query, but it's rationale for failing to execute the query seems pretty solid.
Tarmo's got a query that has some chance of working, so anyone who feels very deeply about this problem should probably work out how to get that query executed for has_many :through with :group, and reopen #10480 (or a new ticket) with a patch that has Tarmo's query and passes on all three databases.
- Matt
I don't think you'd have to generate it just for PgSQL -- that query looks like it should have a good chance of working on all SQL engines (not that I know much about the intricacies of that world). Making has_many :through use that query for getting the relevant records when :group is specified wouldn't be that hard, surely?
- Matt
Unfortunately "DISTINCT ON" is a postgresql extension not standard SQL if mysql works then I'm quite sure the results are unpredictable, just like when using "DISTINCT ON" with postgresql without specifying order.
DISTINCT ON basically retreives only the first row for each distinct value in the list specified after "DISTINCT ON", so "DISTINCT ON ( post_id )" retreives the first result for each unique post, without specifying ORDER the first row is random (determined sometimes by the physical order of rows in the database). But with specifying order one could for example fetch the first comment for each post.
Does mysql with "GROUP BY" behave in the same way?
> > > > Name: test_group_has_many_through_should_use_group_for_count(AssociationsJoinModelTest) > > > > Type: Error > > > > Message: ActiveRecord::StatementInvalid: PGError: ERROR: column "comments.id" must appear in the GROUP BY clause or be used in an aggregate function > > > > > > > > : SELECT "comments".* FROM "comments" INNER JOIN posts ON comments.post_id = posts.id WHERE (("posts".author_id = 1)) GROUP BY comments.post_id > > > > ./test/cases/../../lib/active_record/connection_adapters/abstract_adapter.rb:151:in `log' > > > > ./test/cases/../../lib/active_record/connection_adapters/postgresql_adapter.rb:407:in `execute_without_counting' > > > > ./test/cases/helper.rb:38:in `execute' > > > > ./test/cases/../../lib/active_record/connection_adapters/postgresql_adapter.rb:787:in `select_raw' > > > > ./test/cases/../../lib/active_record/connection_adapters/postgresql_adapter.rb:774: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/base.rb:531:in `find_by_sql' > > > > ./test/cases/../../lib/active_record/base.rb:1245:in `find_every' > > > > ./test/cases/../../lib/active_record/base.rb:502:in `find' > > > > ./test/cases/../../lib/active_record/associations/has_many_through_association.rb:149:in `find_target' > > > > ./test/cases/../../lib/active_record/associations/association_proxy.rb:132:in `load_target' > > > > ./test/cases/../../lib/active_record/associations/association_proxy.rb:123:in `method_missing' > > > > ./test/cases/../../lib/active_record/associations/has_many_through_association.rb:142:in `method_missing' > > > > ./test/cases/associations/join_model_test.rb:607:in `test_group_has_many_through_should_use_group_for_count' > > > > ./test/cases/../../lib/../../activesupport/lib/active_support/testing/setup_and_teardown.rb:59:in `__send__' > > > > ./test/cases/../../lib/../../activesupport/lib/active_support/testing/setup_and_teardown.rb:59:in `run' > > > > > > Further investigation has revealed that this failure is a known issue, and > > > the original patch author has asked for it to be reverted because they can't > > > get it to work on PgSQL. See http://dev.rubyonrails.org/ticket/10480 for > > > details. I can't work out how to make PgSQL play nicely either; I'm in > > > favour of reversion, too. > > > > Not sure but I think this is the closest PostgreSQL query that should > > give the expected effect although without an ORDER BY the results > > are unpredictable. > > > > SELECT DISTINCT ON ( comments.post_id ) "comments".* FROM "comments" > > INNER JOIN posts ON comments.post_id = posts.id WHERE > > (("posts".author_id = 1)); > > > > So instead of "GROUP BY comments.post_id" you should use > > "DISTINCT ON ( comments.post_id )". But I have no idea how easy it would > > be to make activerecord generate this query when using postgresql. > > I don't think you'd have to generate it just for PgSQL -- that query looks > like it should have a good chance of working on all SQL engines (not that I > know much about the intricacies of that world). Making has_many :through > use that query for getting the relevant records when :group is specified > wouldn't be that hard, surely?
Unfortunately "DISTINCT ON" is a postgresql extension not standard SQL if mysql works then I'm quite sure the results are unpredictable, just like when using "DISTINCT ON" with postgresql without specifying order.
Aaah bugger. I thought it was just a different form of "DISTINCT". Drat on database vendors and their extensions.
It'll have to be db-specific then. That'll make my forays into the query generation code just that much more entertaining...
DISTINCT ON basically retreives only the first row for each distinct value in the list specified after "DISTINCT ON", so "DISTINCT ON ( post_id )" retreives the first result for each unique post, without specifying ORDER the first row is random (determined sometimes by the physical order of rows in the database). But with specifying order one could for example fetch the first comment for each post.
Does mysql with "GROUP BY" behave in the same way?
I didn't take a particularly deep look at what MySQL does under the circumstances. I think the test didn't really exercise that part of things, and I haven't yet gotten my head around what exactly is supposed to happen when you ask for :group.
- Matt
First of all, many thanks to everyone for looking into this.
My take is that if you cannot or will not make the tests pass for some edge case (and I'm defining PgSQL as an edge case, for better or worse), and won't roll back the change, then you should disable the test for that edge case.
This would mean adding some conditional logic that just doesn't run this test against PgSQL, but still runs it for all other databases. If you prefer, this is accompanied with some TODO's in the code and a loud stderr message that reminds everyone of the disabled test, but doesn't cause the entire suite to fail.
Yes, this is NOT optimal, but as you said, a constantly failing CI is much worse. It masks other severe, non-edge-case breakages, and makes people ignore the error messages. I call this "Crying Wolf". There are many REAL wolves lurking out there, and if something CAN be ignored for now, it's not really a wolf.
This particular situation is a perfect example of why you NEED CI - to catch edge cases that people don't run local tests for. The next, and much harder step, is to deal with the breakages IMMEDIATELY. Either roll back the change, or disable the test. Neither is great, but both are much better than a constantly flashing red siren that that everybody just ignores.
-- Chad
This particular situation is a perfect example of why you NEED CI - to catch edge cases that people don't run local tests for. The next, and much harder step, is to deal with the breakages IMMEDIATELY. Either roll back the change, or disable the test. Neither is great, but both are much better than a constantly flashing red siren that that everybody just ignores.
Agreed. In this particular case the GROUP BY in a select statement without an aggregate doesn't seem to make sense to me. Apparently it doesn't make sense to postgresql either :).
I'd prefer not to disable tests unless it's something that the adapter *can't* support, rather than 'currently unimplemented'.