Add support for "application intent read only"

[aharpervc]

Add support for "Application Intent Read Only" (https://github.com/rails-sqlserver/tiny_tds/issues/184). The use case is, when connecting to an availability group listener that has read only secondary replicas, the app can request to be connected to a replica rather than the primary.

Other than confirming that there's no regressions and the bit can be set, I'm not sure exactly how to write tests for this.... setting up multiple sql server docker containers just to set up an AG just to test this seems like overkill.

[y310]

@metaskills Hi, I think this change is needed to use recently released read-only replicas on Azure SQL DB. Is there any problems to prevent from this PR get progress?

https://docs.microsoft.com/en-us/azure/sql-database/sql-database-read-scale-out

[aharpervc]

Is there any problems to prevent from this PR get progress?

The "problem" is that I still need to get a test lab set up to confirm this actually works as intended. I haven't had the capacity to revisit that since writing this commit.

It actually would be very helpful if you can test this on your end (check out this branch, build your own copy of the gem, connect to your AG, do the appropriate diagnostics to confirm you're getting routed to the correct replica and otherwise everything seems correct).

[ryu1kn]

We wanted to use read-only intent to connect to our on-premise SQL Server database cluster.

I wrote a snippet to check whether the query is executed on the intended server, and confirmed the SQL is run on the right server. The catch is, I also had to specify contained: true as a workaround to specify the database upon opening a connection.

https://gist.github.com/ryu1kn/a76694c42432cfd62e9fdf16cb2f5c88

[ryu1kn]

Without this PR, we cannot tell FreeTDS to use read-only intent from our Ruby code. One way to get around this is to specify it in freetds.conf file like this.

[my-db]
        host = mydb.domain.com
        port = 1433
        tds version = 7.3
        read-only intent = yes

Then specify my-db in host property when instantiating TinyTDS.

client = TinyTds::Client.new(
  host: 'my-db',
  username: 'db_username',
  password: 'db_password',
  database: 'db_databasename',
  contained: true,
  message_handler: Proc.new { |m| puts m.message }
)

Here, if we move the database name from ruby code to freetds.conf, we can remove contained flag.

[aharpervc]

Alright, I had some time to revisit this branch. I have an AG lab set up locally to verify that setting the new option does indeed work as expected.

There's some nuance to this:

• per @saurabh500's comment here, support was only added for this bit in TDS 7.4. Therefore, I added a warning if you set :read_only_intent and are using some prior version
• per @ryu1kn's comments, the reason why you have to set :contained is that that the code path that sets the login database is only processed when you also pass :azure or :contained. Not sure why it was done that way ¯\(ツ)/¯. However, I have updated that code path to read in the provided default database when passing :read_only_intent as well
• This required also updating the version table to add a 7.4 entry. I also updated the default, but that might not be desirable and we can back it out if so. That'd mean that if you want to use :read_only_intent, you also need to initialize with :tds_version => '7.4'
• Since we don't have an AG in the testing setup, there's not really a good way to verify that things all work. However I have still added an test that you can opt-in to running, by setting TINYTDS_READ_REPLICA_SERVER_NAME. Then run it like TINYTDS_READ_REPLICA_SERVER_NAME=mssql_2 m test/client_test.rb:74 and it'll verify that TinyTDS got redirected to the read replica you told it to expect. This works for me locally and if anyone that can run these tests in their AG, that'd be great as well.

Next step is probably to get a consensus one way or the other about the 7.4 configuration, rework the branch commit messages, and get a code review from anyone available.

[aharpervc]

This should maybe change from a bool to a symbol enumeration, to support Application Intent Read Write https://docs.microsoft.com/en-us/sql/database-engine/availability-groups/windows/secondary-replica-connection-redirection-always-on-availability-groups?view=sql-server-ver15

[aharpervc]

This should maybe change from a bool to a symbol enumeration, to support Application Intent Read Write

The config option is now called :application_intent and supports a value of :read_only to set the corresponding login bit. This more closely matches other libraries & microsoft's documentation.

Building & testing locally with an AG w/ read replicas confirms that this still works (ie, tiny tds is talking to the replica host when application_intent: :read_only is provided).

Next step is probably to get a consensus one way or the other about the 7.4 configuration, rework the branch commit messages, and get a code review from anyone available.

👆 this is still the case

[gisborne]

It sounds like this work is just about done. Why is this issue still open?

[aharpervc]

It sounds like this work is just about done. Why is this issue still open?

Historically on this project we have released features in accordance with needs from users. At my company, we decided to choose a replication technology besides Availability Groups. Therefore, this feature ended up unrequired for us and I suspended working on the PR. Also, as you can see there were no additional responses to my request for consensus about next steps.

However, the PR has remained open in case someone else voices a need for this feature or would like to contribute to it.

[david-alex-wynn]

Hello,

I am fairly green to the open source community, and am not sure if this is the proper way to communicate this, but we could really use this feature. I pulled in your branch and tested it out. Everything appears to be working exactly as we would expect. Is there anything I can do to help get this merged?

[metaskills]

Closing due to age and cleaning up my PR inbox. Feel free to open, talk, etc.

[dhockett-dmp]

I'll jump in and say this would be highly valuable to us.

With regards to the version default, as this seems to be a fairly specific use case, I think it's reasonable that gem consumers would need to specify the new version in order to access the feature. So if there are concerns with bumping the default, the additional requirement of directly specifying :tds_version=>7.4 doesn't sound like a blocker to me.

Is there anything else in particular that's required to resurrect this?