DASSL / ClassDB

An open-source system to let students experiment with relational data
https://dassl.github.io/ClassDB/
Other
8 stars 2 forks source link

Add functions to get and compare server versions #232

Closed smurthys closed 6 years ago

smurthys commented 6 years ago

This PR adds the following functions:

I am still working on a test script for these changes.

Closes #231

smurthys commented 6 years ago

I just pushed changes that test the new helper functions. I welcome your review.

wildtayne commented 6 years ago

All of the tests pass on my system, and the function work well overall. I found one issue that I think is best addressed in getServerVersion. On my Postgres distribution from Ubuntu, getServerVersion() returns '10.3 (Ubuntu 10.3-1)'. This causes an error when using compareServerVersion(): ERROR: invalid input syntax for integer: "3 (Ubuntu 10". It seems the best solution would be to have getServerVersion() throw out anything after it extracts the server version. I don't think we need to have compareServerVersion() handle this specifically, because we can document that it expects a normally formatted version number as input.

smurthys commented 6 years ago

Thanks @srrollo for sharing your Ubuntu results. I am testing with pg 9.6.3 on Windows 10. I wonder if a later version of pg formats the result differently in that it includes OS information.

@KevinKelly25 are you able to test the changes in this PR in your environment? I assume you have pg9.3 and 10.4 on Windows.

I agree with @srrollo's suggestion that getServerVersion should remove any extra info. I'll await @KevinKelly25's response before designing a solution.

wildtayne commented 6 years ago

I just tested my BigSQL 9.6.9 installation, and getServerVersion() returns 9.6.9. I suspect (Ubuntu 10.3-1) is "compiled in" to the Ubuntu version, and is not strictly related to the OS. This is pretty common on software provided in Linux distro repos, in order to show that it is software version X compiled by distro Y for package version Z.

KevinKelly25 commented 6 years ago

For 10.4 and 9.3 the tests all pass with no issues.

afig commented 6 years ago

I tested this with Postgres 9.6 on Windows and on a Ubuntu instance and they worked correctly. Unlike @srrollo's Ubuntu installation, the instance I have installed came from Postgres' own distribution, rather than Ubuntu's. This would agree with description that @srrollo gave about packaging.

smurthys commented 6 years ago

Thank you all for reporting the results from your installations.

Can we safely say that a custom distro returns a value of the form x.y.z (os info) in that we can safely stop if/when we see an open parenthesis?

smurthys commented 6 years ago

I just pushed changes to both the helper functions and the tests to account for distro suffix in version number. With these changes, both getServerVersion and compareServerVersion consider only the portion before opening parenthesis in a retrieved/supplied version number.

KevinKelly25 commented 6 years ago

Tests for version 9.3 and 10.4 still working. @smurthys thanks for the useful helper functions and tests

smurthys commented 6 years ago

I see the test script has an issue: L195 in testHelpers.sql needs some attention. I will look at it in an hour or so.

smurthys commented 6 years ago

The change I just pushed should address the issue on L195 in testHelpers.sql. Before this change that line would have failed in 3rd party distros.

smurthys commented 6 years ago

I just pushed a few changes which collectively do the following:

I believe at this point the changes in this PR pretty much define the PR's scope.

smurthys commented 6 years ago

I agree we need to check the distros @srrollo lists.

We should probably also list the distros we have tested on, or we should specifically say we have tested only on 1st party Linux and Windows distros.

smurthys commented 6 years ago

Thank you all for reviewing.