RubyOnRails build 8789 failed

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