"Invalid cursor state" after calling Exec with multiple INSERT/UPDATE statements on a SQL Server database

[sponeil]

I believe the invalid state is caused by the query returning multiple "affected row" counts.

Steps to reproduce: 1) Download and install any SQL Server Express Edition (free). 2) Create an empty database with any name you like. 3) Update the "Server=" and "Database=" parts of the connection string in the attached test file. 4) Run it. I have run it on a local database and verified that it fails on the second Exec call with the cursor in an invalid state.

odbc_test.txt

[alexbrainman]

@sponeil thank you for providing a repro. I will try and investigate this over the weekend.

Alex

[alexbrainman]

Sure, I can reproduce the problem on Windows. I do not know what the problem is, but Adjusting your code, like this

    query := `
 DECLARE @id INT, @a VARCHAR(255)
 SELECT @id = ?, @a = ?
-UPDATE my_table SET a = @a WHERE id = @id
-IF @@ROWCOUNT = 0
+IF EXISTS(SELECT * FROM my_table WHERE id=@id)
+  UPDATE my_table SET a = @a WHERE id = @id
+ELSE
   INSERT INTO my_table (id, a) VALUES (@id, @a)
 `

fixes the problem here. Will that work for you?

Alex

[sponeil]

That would work for the simple query example I gave you, but I have more complex queries I would like to run without having to worry about this problem.

BTW, there is another workaround in T-SQL, which is to "SET NOCOUNT ON". I'm using it now, but it can have other undesirable effects.

I believe an actual fix should be fairly simple. Just call SQLMoreResults after SQLExecute in a loop until it returns SQL_NO_DATA. That should put the statement handle back in the proper state for the next SQLExecute call. There's a little more to it than that, as each result in a batch of them could have an error.

Here are the docs for SQLMoreResults: https://docs.microsoft.com/en-us/sql/odbc/reference/syntax/sqlmoreresults-function?view=sql-server-2017

The actual C code may look something like this (pseudo-code-ish, may not compile or run exactly like this):

// Results (which can be handled any way you like, and may or may not be cumulative) const char *error = NULL; SQLLEN count = 0;

// Call to SQLExecute SQLRETURN rc = SQLExecute(stmt);

// Handle initial result plus any other results hiding behind the first one. do { if(SQL_SUCCEEDED(rc)) { // NOTE: Not sure if you do anything with warnings given with SQL_SUCCESS_WITH_INFO. SQLLEN c; rc = SQLRowCount(stmt, &c); if(SQL_SUCCEEDED(rc)) { count += c; // If we can't return separate counts, maybe we should return a sum of all counts? } } else { // NOTE: You can choose to only keep the first error, build a list of errors, etc. error = my_error_handling_func(stmt, rc); } rc = SQLMoreResults(stmt); } while(rc != SQL_NO_DATA);

// All done, return error and/or count.

On Sat, Nov 3, 2018 at 1:26 AM Alex Brainman notifications@github.com wrote:

Sure, I can reproduce the problem on Windows. I do not know what the problem is, but Adjusting your code, like this

query := ` DECLARE @id INT, @a VARCHAR(255) SELECT @id = ?, @a = ? -UPDATE my_table SET a = @a WHERE id = @id -IF @@ROWCOUNT = 0 +IF EXISTS(SELECT * FROM my_table WHERE id=@id)

• UPDATE my_table SET a = @a WHERE id = @id +ELSE INSERT INTO my_table (id, a) VALUES (@id, @a) `

fixes the problem here. Will that work for you?

Alex

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/alexbrainman/odbc/issues/127#issuecomment-435561965, or mute the thread https://github.com/notifications/unsubscribe-auth/AT6ZCtPlv-J3iaGCvfcZzZl4vQMZgjH-ks5urSkLgaJpZM4YB5OK .

[sponeil]

Sorry, I hadn't actually even peeked at the source code for your ODBC library. I thought it may have C code that needed to be compiled, and I didn't have time to look at it during the week either. It turned out to be a lot simpler to tweak and test than I'd thought it would be.

Can you see the attached diff.txt? I have no idea if it would break anything on the Unix side, and I haven't tested the Rows.NextResultSet() method I added (though it may be useful to me later). These changes seem to solve my Exec() problem (running on Windows) with multiple queries in it, but it probably needs to be tested more thoroughly before being accepted as a fix.

(Let me know if the attached file doesn't come through, and I will send it another way.)

Sean

EDIT: Attaching the file from the web site: diff.txt

[alexbrainman]

It turned out to be a lot simpler to tweak

Yes, it should not be hard.

Can you see the attached diff.txt?

Yes, I can. It would be easier for me, if you can create Pull Request instead. I can comment on every line of PR. We can submit PR once it is ready. But, if you cannot do PR, diffs are fine too. Let me know, if you need help with PR.

I have no idea if it would break anything on the Unix side, ...

I will test that.

and I haven't tested the Rows.NextResultSet() method I added

Yes, we need new tests to confirm new functionality works. Please, copy some test in mssql_test.go and adjust it as you see fit.

Also, please, try not to change any existing public methods - like renaming Exec into PreExe. I hope noone calls that code directly. But if they do, your change will break their code. Keep code the same, if possible.

I won't test / review your code until you add tests.

Thank you.

Alex

[sponeil]

Sorry, I would definitely do that for you if I had more free time myself. Maybe if I have some down time after this deadline I'm working toward, or maybe during one of the holiday breaks. I imagine you're in the same boat as I am in that respect. For now I'm okay with merely having the problem, workarounds, and a potential fix documented.

On Sat, Nov 3, 2018 at 6:37 PM Alex Brainman notifications@github.com wrote:

It turned out to be a lot simpler to tweak

Yes, it should not be hard.

Can you see the attached diff.txt?

Yes, I can. It would be easier for me, if you can create Pull Request instead. I can comment on every line of PR. We can submit PR once it is ready. But, if you cannot do PR, diffs are fine too. Let me know, if you need help with PR.

I have no idea if it would break anything on the Unix side, ...

I will test that.

and I haven't tested the Rows.NextResultSet() method I added

Yes, we need new tests to confirm new functionality works. Please, copy some test in mssql_test.go and adjust it as you see fit.

Also, please, try not to change any existing public methods - like renaming Exec into PreExe. I hope noone calls that code directly. But if they do, your change will break their code. Keep code the same, if possible.

I won't test / review your code until you add tests.

Thank you.

Alex

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/alexbrainman/odbc/issues/127#issuecomment-435626284, or mute the thread https://github.com/notifications/unsubscribe-auth/AT6ZCh79arVXtXhmPri9XWv5nj5Dr3J8ks5urhqVgaJpZM4YB5OK .

[alexbrainman]

For now I'm okay with merely having the problem, workarounds, and a potential fix documented.

I am not sure, what documentation you are referring. You have a problem. And you have potential fix https://github.com/alexbrainman/odbc/issues/127#issuecomment-435593043. Just tests are missing. If you have no time to implement it, you would have to wait for someone else to do it.

Alex

[sponeil]

This bug report itself documents the problem, workarounds, and a proof-of-concept fix that should help anyone else who runs into it.

My proof-of-concept fix was a quick hack, and it didn't take much time at all. Cleaning it up and writing (and testing) unit tests would take even more time, and unfortunately I don't have any right now.

I have a workaround. It's not ideal, but I have bigger fish to fry, so for now it's good enough.

On Mon, Nov 5, 2018 at 6:56 PM Alex Brainman notifications@github.com wrote:

For now I'm okay with merely having the problem, workarounds, and a potential fix documented.

I am not sure, what documentation you are referring. You have a problem. And you have potential fix #127 (comment) https://github.com/alexbrainman/odbc/issues/127#issuecomment-435593043. Just tests are missing. If you have no time to implement it, you would have to wait for someone else to do it.

Alex

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/alexbrainman/odbc/issues/127#issuecomment-436080656, or mute the thread https://github.com/notifications/unsubscribe-auth/AT6ZCn3sDgbFPVQ5G9f40T5GqOd7GufEks5usNAWgaJpZM4YB5OK .

[alexbrainman]

I have a workaround. It's not ideal, but I have bigger fish to fry, so for now it's good enough.

SGTM. Thanks for explaining.

Alex