Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sum rows affected for multi-statement Exec calls. #198

Merged
merged 1 commit into from
Jan 11, 2017
Merged

Sum rows affected for multi-statement Exec calls. #198

merged 1 commit into from
Jan 11, 2017

Conversation

joliver
Copy link
Contributor

@joliver joliver commented Dec 22, 2016

When a given Exec call contains multiple SQL statement, we should sum the number of rows affected from all the statements executed rather than using the number of rows affected from the last statement, e.g.

UPDATE Timer SET unit = 1 WHERE id = 1234 # no rows affect if the row doesn't exit, otherwise 1 row affected

INSERT INTO Timer (id, unit)
SELECT 1234, 1
   FROM Timers
 WHERE @@rowcount = 0 # if the above statement affects rows, this will return no rows affected

By summing all the rows affected in the Go code, you get the count of what happened.

@heyimalex
Copy link
Contributor

I'm not sure if that's more intuitive? Many people who write sql know that @@rowcount only applies to the last query, so changing RowsAffected to return the sum is sort of unexpected. Just my 2c

@joliver
Copy link
Contributor Author

joliver commented Dec 22, 2016

Agreed that most people don't use @@rowcount which is a bit of a special case, but doing multiple operations in a single Exec is common enough and each statement has the potential to affect rows, so only returning the last statement's affected rows isn't giving the complete story about what happened.

@heyimalex
Copy link
Contributor

Hm, I think I wasn't clear. I was saying that @@rowcount the tsql statement only returns the last row count, and maybe people would expect RowsAffected to do the same thing. On the other hand, sql server management studio shows the total for the whole request.

Grepping through our code at work, I always use SELECT @@rowcount to grab the rowcount when I want it, so this wouldn't affect me personally. But it is potentially a breaking change if anyone relied on the old behavior.

@joliver
Copy link
Contributor Author

joliver commented Dec 23, 2016

True it might have the potential to break a few people who happen to be relying on the current behavior. That said, the way number of rows affected is counted hasn't been clearly defined in any documentation, so anyone relying on that up to this point is doing so by coincidence.

The other mssql Go driver based which uses CGO returns total number of rows affected.

Also, the idea of changing behavior without changing the signature happens all the time from the Go maintainers working on the Go standard library. In the various discussions they decide that XYZ behavior hasn't been fully defined so they define it in the docs and change the behavior. These kinds of breaking changes are typically found in release notes so people can find them.

I can make things work without a change, but it's at least worth considering.

@kardianos
Copy link
Collaborator

I'm in favor of this change. If we do it we should do it before go1.8 is out.
Optionions @denisenkom @dimdin ?

@kardianos
Copy link
Collaborator

I think the docs are clear in this aspect:

    // RowsAffected returns the number of rows affected by an
    // update, insert, or delete. Not every database or database
    // driver may support this.

In other words, we for multiple results, or multiple statements even within a single batch should be summed. No other opinions have been voiced, so I will be merging in.

@kardianos kardianos merged commit 370263e into denisenkom:master Jan 11, 2017
gabrielcorado pushed a commit to gravitational/go-mssqldb that referenced this pull request Oct 4, 2024
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