MsSql query_runner: Commit the transaction for inserts & Updates

[mattdjones]

When running a ms sql stored procedure that updates or inserts data the update/insert was not happening.

As discussed here https://discuss.redash.io/t/insert-or-update-query-does-not-apply-in-sql-server/1233
It was suggested to commit the transaction.

We have made the change to our local installation, it would be great if we can have this included.
@arikfr

[mattdjones]

@RichardLitt @arikfr Is there anything I can do to help get this pr merged?

It would be really cool if you can include it in a release we have to manually change the code if we want to spin up a new instance/server.

[mattdjones]

hey @RichardLitt & @arikfr

Can I do anything more to get this merged? we have to run a custom script to add the commit for the transaction in our deployment.

[mattdjones]

hey @arikfr is this PR good to merge?

[mattdjones]

hey @arikfr we still make this code modification in our self hosted installation so that we can execute write queries, is there another configuration option we are missing that will let us do the same thing we are missing somewhere?

[susodapop]

Thanks for drawing attention to this again. I apologise it's taken this long for you to receive a response :( We're working to improve our turnaround time and community engagement 🙏

Two items:

• The mssql.py query runner has been deprecated since V9. We replaced it with the odbc-based version. We aren't making any changes to the old one at this point. Would it make more sense to apply this change to the new odbc-based query runner?
• Redash is really meant to run SELECT statements. Executing stored procedures is outside the normal scope we target. Will committing at the end of every query result in any changes for users running SELECTs?

[mattdjones]

thanks @susodapop - I posted this reply before I found out about the runner deprecation :) so yes for us it would make sense to alter the newer runner.

Will committing at the end of every query result in any changes for users running SELECTs - that's a good question, for ms sql it slightly depends on the individual installation, but in general it would not make any difference.
Most of the time ms sql will use Autocommit mode by default, so every tsql statement is committed or rolled back when completed, but selects have nothing to commit.

We find it a really useful feature to be able to give limited write access to restricted data. Would it make sense to add it as an optional setting to commit the transaction explicitly?

[guidopetri]

@mattdjones , thanks for the continued interest in getting this merged. Now that Redash is Community-driven, I'm trying to emphasize getting older PRs merged in (or closed, if the authors aren't interested anymore).

If you want to change the ODBC runner, then please let me know :) I think this would work as an admin-level setting, since like susodapop mentioned previously, Redash isn't focused on anything other than select statements. Maybe this could be an environment variable that can be enabled/disabled when spinning up the instance?

[mattdjones]

hey @guidopetri thanks for following up, I wasn't sure how active this repo is but i'm glad to see you're trying to progress things :)

I saw a new fork was started, but is work continuing here?

I know updating the odbc runner is something that would help our use case 👍

I think this would work as an admin-level setting, do you mean a global setting for users of an admin group?

[justinclift]

I saw a new fork was started, but is work continuing here?

Yeah, we started doing stuff in the RedashCommunity/redash fork as we didn't have the right permissions to get stuff done here.

However, about 2 weeks later we got the right permissions to do stuff here after all. 😁

So we switched back to this repo and have been busy getting it back into shape. eg Merging issues, fixing a bunch of outdated pieces (still ongoing), etc. 😄

[justinclift]

The I think this would work as an admin-level setting, confuses me too.

If Redash is automatically starting a transaction before it runs the SELECT, then adding an extra commit there shouldn't hurt things for any situation should it?

At least, nothing is springing to mind at the moment. 😄

[codecov]

Codecov Report

Merging #2538 (7efc6dd) into master (177f33a) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #2538      +/-   ##
==========================================
- Coverage   60.02%   60.02%   -0.01%     
==========================================
  Files         153      153              
  Lines       12494    12495       +1     
  Branches     1692     1692              
==========================================
  Hits         7500     7500              
- Misses       4781     4782       +1     
  Partials      213      213              

Files Changed	Coverage Δ
redash/query_runner/mssql.py	32.09% <0.00%> (-0.41%)	⬇️

[justinclift]

Hmmm, it'd be optimal if we could add some kind of test to this PR, to make Codecov happy. @mattdjones if you're not sure how to do that, then maybe @konnectr would have ideas (?). 😄

[guidopetri]

TBH, I'd be good merging this in as-is - this section of the code isn't covered by tests currently anyway

[justinclift]

@guidopetri No worries. Lets get it done then. 😄