ReaderModificationCommandBatch and affected rows

[roji]

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.

Looking 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().

PostgreSQL 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 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:

• 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.
• 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.
• You should probably consider making SqlGenerator.AppendSelectAffectedCountCommand virtual rather than abstract.

Thanks for any advice on this...

[rowanmiller]

@AndriySvyryd can you give some input on this one?

[rowanmiller]

@AndriySvyryd - ping... can you take a look at this

[AndriySvyryd]

Hello roji,

Sorry for late response, I was on vacation.
The reason we use SELECT @@ROWCOUNT is that we need to be able to provide enough information for handling optimistic concurrency issues in batches. E.g. if 2 update and 3 delete operations are executed in a single DbCommand and only 4 rows were affected ideally we should be able to tell which operation failed without any additional queries.

I haven't worked with PostgreSQL, but won't the following snippet achieve the desired result?

GET DIAGNOSTICS row_count = ROW_COUNT;
SELECT row_count;

The 'ReaderModificationCommandBatch' implementation expects this behavior because I assumed that most SQL stores either had a way to return the number of rows affected by previous statement or didn't support batches at all.

If that's not the case for PostgreSQL and some other stores we could add a ModificationCommandBatch that uses IDbDataReader.RecordsAffected and provides limited support for optimistic concurrency handling. Or better yet we could accept a contribution for this.

If there are other places where you had to reimplement functionality from the relational provider because of poor factoring, please let us know.

[roji]

@AndriySvyryd thanks for your reply.

Unfortunately, in PostgreSQL GET DIAGNOSTICS is only supported in plpgsql, and not in regular SQL. I think that means we can't rely on it (it may be disabled etc.). This seems to be listed as an unsupported feature of PostgreSQL, feature F121-01

However, the affected rows information is definitely available on a command-by-command basis in the PostgreSQL protocol, in the CommandComplete message - Npgsql's implementation of IDbDataReader.RecordsAffected uses that. The only issue is that IDbDataReader.RecordsAffected aggregates these values.

It seems that best solution would be to add a non-standard method in Npgsql which would expose the unaggregated numbers. I've opened npgsql/npgsql#585 for this and will implement.

Regarding providing a ModificationCommandBatch that only uses IDbDataReader.RecordsAffected, I guess I agree with you assumption that if a database provides batching, it's going to provide some way to access each command's affected rows... It's just not necessarily going to do that via an SQL query such as SELECT @@ROWCOUNT...

[AndriySvyryd]

Ok, so if we make ReaderModificationCommandBatch.ConsumeResultSet* and AggregateEntries methods protected virtual, make _valueReaderFactory a protected property and make SqlGenerator.AppendSelectAffectedCountCommand virtual would that be enough granularity?

[roji]

@AndriySvyryd am hard at work at a minimal working provider for a //BUILD demo so I'll get around to this next week...

[divega]

@AndriySvyryd should we make this a pri0 given the request from PostgreSQL and SQL CE?