Skip to content

Conversation

@andyundso
Copy link
Member

With this PR, we drop support for SQL Server < 2017.

The first part of the PR extends our existing Continous Integration to run tests against each combination of our supported Ruby versions and SQL Server 2017, 2019 and 2022 on both Linux and Windows.

I therefore would also suggest we remove support for anything below SQL Server 2017. Technically, people could still use tiny_tds against these older versions as long as they compile FreeTDS with TDS v7.3 or v7.4. And SQL Server 2008 is the earliest version that supports v7.3. However, I think it's good if we can test our software against what we support, and if 2017 is the earliest version where I can do this, then I'm happy to drop support for anything older. Only v2014 and v2016 are still covered by extended maintenance of Microsoft, we can expect most people upgraded at this point.

The second part of this PR is code removal. First of all, there was not any new data type or changes to them since SQL Server 2016, so I removed all the different schemas. I also removed two ifdef from the C code.

  • DBSETUTF16 is there to detect if UTF-16 is supported by FreeTDS. This was introduced in v1.0, and I think we can expect everybody to run a newer version than v1.0.
  • DBVERSION_73 detects if FreeTDS supports TDS v7.3, which is the case since v0.95. We mainly used it to cast the different date and time formats in these newer TDS versions. Again, I think it's reasonable to expect that everybody runs a newer version than v0.95.

People should get a compile error when trying to build tiny_tds now with an version older than v1.0 when building tiny_tds.

Two things I'd like to tackle in the future.

  • I would like to run the test suite against Azure, since this is a test case where I see it will be hard to test it in the pipeline, but testing it manually from time to time should be fine.
  • Setup MSSQL with encryption in the pipeline. Maybe just for one version (2022?) to also verify that our OpenSSL setup works.

@andyundso andyundso requested a review from aharpervc March 28, 2024 20:38
Copy link
Contributor

@aharpervc aharpervc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, love all the deletions! I left a few minor line comments then LGTM

image

README.md Outdated
```

This will download our SQL Server for Linux Docker image based from [microsoft/mssql-server-linux/](https://hub.docker.com/r/microsoft/mssql-server-linux/). Our image already has the `[tinytdstest]` DB and `tinytds` users created. This will also download a [toxiproxy](https://github.com/shopify/toxiproxy) Docker image which we can use to simulate network failures for tests. Basically, it does the following.
This will download our SQL Server for Linux Docker image based from [microsoft/mssql-server-linux/](https://hub.docker.com/r/microsoft/mssql-server-linux/). Our image already has the `[tinytdstest]` DB and `tinytds` users created. This will also download a [toxiproxy](https://github.com/shopify/toxiproxy) Docker image which we can use to simulate network failures for tests. Basically, it does the following:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be rephrased/removed now that we're using the official image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed with 17d0fea.

services:
mssql:
image: mcr.microsoft.com/mssql/server:2017-latest
image: mcr.microsoft.com/mssql/server:${MSSQL_VERSION:-2017}-latest
Copy link
Contributor

@aharpervc aharpervc Mar 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is now the time to also bump the default version here? (after reading the rest of the PR), eh probably fine as is

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see it the same way: We can bump it again once it's time to drop 2017 support :)

- restore_cache:
name: restore mssql installation file
key: downloads-{{ checksum "test/bin/install-mssql.ps1" }}
key: downloads-<< parameters.mssql_version >>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend keeping the checksum, even if you want to also introduce the mssql version to the key

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added with aa830a4.

use utf-16 = true
```

The default is true and since FreeTDS v1.0 would do this as well.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you fold this sentence into the above paragraph? If it's the default then no config file is required to get utf 16 working

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed with bf00434.

README.md Outdated
```
$ rake TINYTDS_UNIT_DATASERVER=mydbserver
$ rake TINYTDS_UNIT_DATASERVER=mydbserver TINYTDS_SCHEMA=sqlserver_2008
$ rake TINYTDS_UNIT_DATASERVER=mydbserver TINYTDS_SCHEMA=sqlserver_2016
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be 2017 since it's basically the new default min version?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed with 6684afb.

case 42: // SYBMSDATETIME2
case 43: { // SYBMSDATETIMEOFFSET
#ifdef DBVERSION_73
if (dbtds(rwrap->client) >= DBTDS_7_3) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm reading this correctly, TDS 7.3 is the new min version, correct? Any benefit to a reverse check somewhere (not necessarily here) that raises an exception if the version is < 7.3?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added two checks with cfe7d92:

  1. If the DBVERSION_73 symbol is not defined, the user is using a very old FreeTDS version and should be unable to compile the code.
  2. Once we established the connection, we can ask about the tds version used to connect. There I added an exception if using a TDS version older than 7.3. This worked quite well.
$ TDSVER=7.2 bundle exec rake test
Basic query and result::with multiple result sets::using :empty_sets TRUE#test_0001_handles a basic empty result set:
TinyTds::Error: connecting with a TDS version older than 7.3!
    /home/andy/dev/andyundso/tiny_tds/lib/tiny_tds/client.rb:60:in `connect'

require 'toxiproxy'

TINYTDS_SCHEMAS = ['sqlserver_2000', 'sqlserver_2005', 'sqlserver_2008', 'sqlserver_2014', 'sqlserver_azure', 'sybase_ase'].freeze
TINYTDS_SCHEMAS = ['sqlserver_2016', 'sqlserver_azure'].freeze
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar question about saying 2016 in this file if the new min version is 2017

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed with 6684afb.

* Drop support for Ruby < 2.7
* Drop support for SQL Server < 2017
* Drop support for FreeTDS < 0.95
* Drop support for FreeTDS < 1.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will 1.0 actually work or is 1.1 the new safe minimum?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reverted locally to an older FreeTDS version we once shipped, 1.00.27:

diff --git a/ext/tiny_tds/extconsts.rb b/ext/tiny_tds/extconsts.rb
index 0a362fa..65d3773 100644
--- a/ext/tiny_tds/extconsts.rb
+++ b/ext/tiny_tds/extconsts.rb
@@ -2,8 +2,14 @@
 ICONV_VERSION = ENV['TINYTDS_ICONV_VERSION'] || "1.15"
 ICONV_SOURCE_URI = "http://ftp.gnu.org/pub/gnu/libiconv/libiconv-#{ICONV_VERSION}.tar.gz"
 
-OPENSSL_VERSION = ENV['TINYTDS_OPENSSL_VERSION'] || '1.1.1s'
+OPENSSL_VERSION = ENV['TINYTDS_OPENSSL_VERSION'] || '1.1.0j'
 OPENSSL_SOURCE_URI = "https://www.openssl.org/source/openssl-#{OPENSSL_VERSION}.tar.gz"
 
-FREETDS_VERSION = ENV['TINYTDS_FREETDS_VERSION'] || '1.1.24'
-FREETDS_SOURCE_URI = "http://www.freetds.org/files/stable/freetds-#{FREETDS_VERSION}.tar.bz2"
+FREETDS_VERSION = ENV['TINYTDS_FREETDS_VERSION'] || "1.00.27"
+FREETDS_VERSION_INFO = Hash.new { |h,k|
+  h[k] = {files: "http://www.freetds.org/files/stable/freetds-#{k}.tar.bz2"}
+}
+FREETDS_VERSION_INFO['1.00'] = {files: 'http://www.freetds.org/files/stable/freetds-1.00.tar.bz2'}
+FREETDS_VERSION_INFO['0.99'] = {files: 'http://www.freetds.org/files/current/freetds-dev.0.99.678.tar.gz'}
+FREETDS_VERSION_INFO['0.95'] = {files: 'http://www.freetds.org/files/stable/freetds-0.95.92.tar.gz'}
+FREETDS_SOURCE_URI = FREETDS_VERSION_INFO[FREETDS_VERSION][:files]

I had three test errors locally with network connection failures through Toxiproxy, but I also encounter these with the newer FreeTDS version. So I would say v1.0 is safe to say we can support.

In case we adjust the script (e.g. update the download links), the cache will be properly invalidated.
Since it is our new minimum version.
was only needed for TDS 5.0 and older.
@andyundso
Copy link
Member Author

@aharpervc would you mind to re-review. besides implementing your feedback, I also optimised two things in the C.

@andyundso andyundso requested a review from aharpervc April 2, 2024 20:39
Copy link
Contributor

@aharpervc aharpervc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@andyundso
Copy link
Member Author

@aharpervc I ran out of free credits on CircleCI thanks to this PR. It is somewhat expected since this PR adds many more parallel pipeline runs, but still happened faster than I anticipated.

I see two options:

  1. Revert most changes here in regards to the pipeline, means we continue to only test on MSSQL 2017.
  2. Move to GitHub Actions since you have unlimited runtime when the repository is public. I already built of copy of our pipeline last year in a fork of ours, so most work is already done, just needs to updated again (drop the older Ruby builds, add 3.3, update rake-compiler-dock).

I personally grew very found of GitHub Actions, mainly because of the actions. At work, we are moving all projects over from Semaphore CI. But it was you in collaboration with @bryanwieg who got the CircleCI pipeline up-and-running, so I would leave the choice going forward to you.

@bryanwieg
Copy link
Contributor

I would not take it personally at all if the pipeline was changed to GitHub. Even when I worked on it so long ago, trying to manage the CircleCI credits was a constant battle. Besides being free, GitLab is a widely used integrated and supported solution, whereas I’ve never again used CircleCI outside this project.

@aharpervc
Copy link
Contributor

But it was you in collaboration with @bryanwieg who got the CircleCI pipeline up-and-running, so I would leave the choice going forward to you.

Also fine with me to set up a PR to switch to GH Actions, for whoever has time to work on it. Not sure if you're saying it needs to be a prerequisite for this PR though, I'd personally prefer that be separate work. But that'd also give us all a chance to try and imagine a CI workflow that's less complicated, if such a thing is possible.

@andyundso
Copy link
Member Author

Not sure if you're saying it needs to be a prerequisite for this PR though, I'd personally prefer that be separate work

Absolutely not, I just can't re-run the one failing test (downloading and installing Ruby on Windows timed out) since I do not have any credits left. But I would go ahead and merge this PR before building the GitHub Actions part next.

@andyundso andyundso merged commit 6f88538 into rails-sqlserver:master Apr 9, 2024
@andyundso andyundso deleted the adjust-sql-server-support branch April 9, 2024 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants