Raise friendly errors when features are used on versions of postgres that don't support them about scenic HOT 8 CLOSED

Example errors from Postgres 9.2:

ERROR:  42601: syntax error at or near "MATERIALIZED"
LINE 1: CREATE MATERIALIZED VIEW foo AS SELECT foo'::text as text;

ERROR:  42601: syntax error at or near "REFRESH"
LINE 1: REFRESH MATERIALIZED VIEW foo

Example Error from Postgres 9.3:

ERROR:  42601: syntax error at or near "CONCURRENTLY"
LINE 1: REFRESH MATERIALIZED VIEW CONCURRENTLY tests;

We can do a regex match on the message. The bigger challenge is going to be testing this. We can try to fake the exception raising, but I wouldn't feel great about that. In reality, we'd need to be able to test against multiple PG versions in our suite (at least on CI). Not sure if that's possible.

from scenic.

I'd be happy to work on this, too!

Based on this closed issue on TravisCI, it doesn't look like they support that functionality: travis-ci/travis-ci#954

Maybe one way around this would be to hook it up to several different CI servers and configure (for example) Travis to run with 9.4, CircleCI to run with 9.3 and something else like Semaphore to run with 9.2. We would probably have to have some sort of conditionals in the tests as well so that they're run or skipped depending on the Postgres version that we're testing against.

Or, if there's another CI service out there that does let you configure multiple versions of a database then you could migrate over to that service if this is a crucial part of the testing. I'd be happy to dig a little deeper and see if that's possible if you like.

from scenic.

Another idea would be to try to get the version from postgres and error based on that rather than the syntax error. I'd feel more comfortable stubbing that. The postgres adapter has a "postgresql_version" method, but unfortunately it's protected.

ActiveRecord::Base.connection.send(:postgresql_version)
# => 90405

Also curious that it returns 90405. I guess it allows for strict numerical comparison which might be nice. For materialized view support we'd check against >= 90300 and for refresh concurrently, we'd check against >= 90400.

Which all sounds well and good... except it's not a public method. Maybe we'd have to re-implement it? I wonder if any other PG gems are doing anything similar?

from scenic.

ActiveRecord::Base.connection.supports_materialized_views? gets us part of the way there. Unfortunately, it was only added in 4.2, and our gemspec says we support 4.1.

I've opened a PR against Rails to make server_version a public method, but even if that were merged it would be Rails 5+ only.

A monkey patch of some sort might be the best solution unfortunately. We're done it before (see ignore_tables on SchemaDumper.

Hrmmm.

from scenic.

I like the idea of returning the error based on the Postgres version instead of parsing the error from ActiveRecord! I think re-implementing might not be the worst thing in the world - especially since it will decouple it from any changes that might be made in Rails in the future, and it's a really tiny re-implementation. How about going around the built in Rails methods and asking Postgres directly for its version? Something like this might work:

ActiveRecord::Base.connection.select_value('select version()') #=> "PostgreSQL 9.4.2 on x86_64-unknown-linux-gnu, compiled by gcc (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3, 64-bit"

We can get the version from there via RegEx and then have our own methods for supports_materalized_views? and supports_concurrent_materalized_views? that we can base the error message off of. I don't think Postgres is going to ever change select version(), and it will also be easy to extend this pattern to future databases.

I'll put together a kind of quick and dirty version of this just to flesh out the idea, and then if you think it would be better to go about it a different way then we can totally do that.

from scenic.

Ok, I've put together some rough ideas about how to tackle this in #111. For now I think this might be a good solution, but I'm going to continue to think about it for the next day or two.

from scenic.

@devonestes This is the right direction, I think. I'm working on #112 which also needs some feature checking based on version. I have an idea for encapsulation of this feature that I might like (which includes some refactor of adapter internals I've wanted to do), so let me post that and see what you and Caleb think about that.

from scenic.

Closing in favor of the PR in progress at #111

from scenic.