{"action":"created","issue":{"url":"https://api.github.com/repos/aspnet/EntityFramework/issues/1956","labels_url":"https://api.github.com/repos/aspnet/EntityFramework/issues/1956/labels{/name}","comments_url":"https://api.github.com/repos/aspnet/EntityFramework/issues/1956/comments","events_url":"https://api.github.com/repos/aspnet/EntityFramework/issues/1956/events","html_url":"https://github.com/aspnet/EntityFramework/issues/1956","id":65342495,"number":1956,"title":"ReaderModificationCommandBatch and affected rows","user":{"login":"roji","id":1862641,"avatar_url":"https://avatars.githubusercontent.com/u/1862641?v=3","gravatar_id":"","url":"https://api.github.com/users/roji","html_url":"https://github.com/roji","followers_url":"https://api.github.com/users/roji/followers","following_url":"https://api.github.com/users/roji/following{/other_user}","gists_url":"https://api.github.com/users/roji/gists{/gist_id}","starred_url":"https://api.github.com/users/roji/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/roji/subscriptions","organizations_url":"https://api.github.com/users/roji/orgs","repos_url":"https://api.github.com/users/roji/repos","events_url":"https://api.github.com/users/roji/events{/privacy}","received_events_url":"https://api.github.com/users/roji/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":{"login":"AndriySvyryd","id":6539701,"avatar_url":"https://avatars.githubusercontent.com/u/6539701?v=3","gravatar_id":"","url":"https://api.github.com/users/AndriySvyryd","html_url":"https://github.com/AndriySvyryd","followers_url":"https://api.github.com/users/AndriySvyryd/followers","following_url":"https://api.github.com/users/AndriySvyryd/following{/other_user}","gists_url":"https://api.github.com/users/AndriySvyryd/gists{/gist_id}","starred_url":"https://api.github.com/users/AndriySvyryd/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/AndriySvyryd/subscriptions","organizations_url":"https://api.github.com/users/AndriySvyryd/orgs","repos_url":"https://api.github.com/users/AndriySvyryd/repos","events_url":"https://api.github.com/users/AndriySvyryd/events{/privacy}","received_events_url":"https://api.github.com/users/AndriySvyryd/received_events","type":"User","site_admin":false},"milestone":{"url":"https://api.github.com/repos/aspnet/EntityFramework/milestones/9","html_url":"https://github.com/aspnet/EntityFramework/milestones/Discussions","labels_url":"https://api.github.com/repos/aspnet/EntityFramework/milestones/9/labels","id":692755,"number":9,"title":"Discussions","description":"<!--\r\n@huboard:{\"order\":101}\r\n-->","creator":{"login":"rowanmiller","id":4741851,"avatar_url":"https://avatars.githubusercontent.com/u/4741851?v=3","gravatar_id":"","url":"https://api.github.com/users/rowanmiller","html_url":"https://github.com/rowanmiller","followers_url":"https://api.github.com/users/rowanmiller/followers","following_url":"https://api.github.com/users/rowanmiller/following{/other_user}","gists_url":"https://api.github.com/users/rowanmiller/gists{/gist_id}","starred_url":"https://api.github.com/users/rowanmiller/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/rowanmiller/subscriptions","organizations_url":"https://api.github.com/users/rowanmiller/orgs","repos_url":"https://api.github.com/users/rowanmiller/repos","events_url":"https://api.github.com/users/rowanmiller/events{/privacy}","received_events_url":"https://api.github.com/users/rowanmiller/received_events","type":"User","site_admin":false},"open_issues":18,"closed_issues":37,"state":"open","created_at":"2014-06-16T19:38:35Z","updated_at":"2015-06-18T03:39:17Z","due_on":null,"closed_at":null},"comments":15,"created_at":"2015-03-31T01:47:02Z","updated_at":"2015-06-23T15:05:57Z","closed_at":null,"body":"I'm working on an EF7 provider for PostgreSQL (with Npgsql), it's my very first foray into the EF codebase so I hope I won't be saying too much nonsense.\r\n\r\nLooking at `ReaderModificationCommandBatch.Execute()`, I notice that it retrieves the affected rows (in the two ConsumeResultSet* methods). It appears to do this by reading the result of a `SELECT @@ROWCOUNT` appended by `SqlServerSqlGenerator.AppendSelectAffectedCountCommand()`.\r\n\r\nPostgreSQL has no equivalent query; affected row counts are sent within the protocol itself, and are currently exposed on the ADO.NET level via the [IDbDataReader.RecordsAffected](https://msdn.microsoft.com/en-us/library/system.data.idatareader.recordsaffected(v=vs.110).aspx) property. In my NpgsqlModificationCommandBatch I can override Execute() and check against that property, but wanted to make sure I'm going down the right path:\r\n\r\n* Is there a special reason to use an SqlServer-specific feature (`SELECT @@ROWCOUNT`) rather than the standard ADO.NET API for this? Does this have to do with the fact that RecordsAffected is only supposed to be populated after the reader is closed, and therefore gives the aggregate rows rather than resultset-by-resultset? Note that the PostgreSQL protocol *does* provide affected rows on a resultset-by-resultset basis, so this information can in theory be exposed to the EF provider if absolutely necessary.\r\n* It seems a bit odd for `ReaderModificationCommandBatch`, a generic class within EntityFramework.Relational to encode logic that is so specific to the SqlServer adapter, while at the same time containing other, more generic functionality.\r\n* You should probably consider making `SqlGenerator.AppendSelectAffectedCountCommand` virtual rather than abstract.\r\n\r\nThanks for any advice on this...\r\n"},"comment":{"url":"https://api.github.com/repos/aspnet/EntityFramework/issues/comments/114537168","html_url":"https://github.com/aspnet/EntityFramework/issues/1956#issuecomment-114537168","issue_url":"https://api.github.com/repos/aspnet/EntityFramework/issues/1956","id":114537168,"user":{"login":"roji","id":1862641,"avatar_url":"https://avatars.githubusercontent.com/u/1862641?v=3","gravatar_id":"","url":"https://api.github.com/users/roji","html_url":"https://github.com/roji","followers_url":"https://api.github.com/users/roji/followers","following_url":"https://api.github.com/users/roji/following{/other_user}","gists_url":"https://api.github.com/users/roji/gists{/gist_id}","starred_url":"https://api.github.com/users/roji/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/roji/subscriptions","organizations_url":"https://api.github.com/users/roji/orgs","repos_url":"https://api.github.com/users/roji/repos","events_url":"https://api.github.com/users/roji/events{/privacy}","received_events_url":"https://api.github.com/users/roji/received_events","type":"User","site_admin":false},"created_at":"2015-06-23T15:05:57Z","updated_at":"2015-06-23T15:05:57Z","body":"OK, I've submitted #2448 which refactors the affect row query logic out from `ReaderModificationCommandBatch` and into what I ended calling `AffectedCountModificationCommandBatch`. The main reason behind this name is to clearly link it to `SqlGenerator.AppendSelectAffectedCountCommand`.\r\n\r\nSome comments:\r\n* I ended up leaving `Execute` as it is (apart from moving it to the `AffectedCountModificationCommandBatch`, breaking it up into two parts seemed overly complicated.\r\n* `SqlServerModificationCommandBatch` and `SingularModificationCommandBatch` now inherit from `AffectedCountModificationCommandBatch`. Very strictly speaking it should be named `SingularAffectedCountModificationCommandBatch` but I already hate myself for introducing such long names.\r\n* ReaderModificationCommandBatch made use of RelationalLoggerExtensions.LogCommand, which is internal so it's not usable from the NpgsqlModificationCommandBatch. Maybe consider making some of those extensions public for providers?\r\n* A design question I'm not sure about... It seems like it's the ModificationCommandBatch implementation's responsibility to check that the affected rows is correct, and to raise a DbUpdateConcurrencyException otherwise. If that's the case, why does the Execute method return the affected rows at all, can't the caller (BatchExecutor) simply look at the batch's ModificationCommand.Count to infer how many rows where affected? Shouldn't Execute either perform all its updates successfully (checking that internally), or raise an exception if it couldn't?\r\n"}}