Use a MAX() aggregation when fetching current count during reconciliation

[reidab]

Fixes #253

On PostgreSQL, if the table where the counts are stored doesn't have a primary_key specified (in our case, because it's actually a view), reconciliation fails with an error:

PG::GroupingError:
   ERROR:  column "foo.users_count" must appear in the GROUP BY clause or be used in an aggregate function
   LINE 1: ...foo".id, COUNT("users".id)*1 AS count, "foo..

This is because, according to the Postgres docs:

When GROUP BY is present, or any aggregate functions are present, it is not valid for the SELECT list expressions to refer to ungrouped columns except within aggregate functions or when the ungrouped column is functionally dependent on the grouped columns, since there would otherwise be more than one possible value to return for an ungrouped column. A functional dependency exists if the grouped columns (or a subset thereof) are the primary key of the table containing the ungrouped column.

When there's no primary key index present, even though Rails knows what the primary key is, no functional dependency gets detected at the DB level.

This MAX agg should effectively be a no-op in every case, but it prevents PG from blowing up. An alternate fix to this same problem would be adding "#{relation_class_table_name}.#{column_name}" to the GROUP BY fields.

Note: the specs added in this commit will pass on SQLite even without these fixes — they work as expected when running the specs on a PostgreSQL DB. Running the tests on PG also reveals that the existing string ID specs (which also don't have a primary_key index) fail in the same way).

[reidab]

Any chance of getting this merged soon? Would it be helpful for me to set up PostgreSQL testing on Travis to demonstrate the issue?

[magnusvk]

Yes, this makes sense, thank you for this. Sorry, I didn't get a notification from Github when you opened this for some reason.

Released this as 2.2.2 right now.

[magnusvk]

Thinking about this some more: Do you think you could get this test running in PostgreSQL also? I think that would be really good to make sure I don't break this for you again in the future.