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

Microsoft.Data.Sqlite: Results affected by subsequent statements when batching #13830

Closed
Grauenwolf opened this issue Sep 7, 2017 · 21 comments

Comments

@Grauenwolf
Copy link

commented Sep 7, 2017

If I execute this command I get 5 rows back.

SELECT [EmployeeKey], [FirstName], [MiddleName], [LastName], [Title], [ManagerKey], [CreatedDate], [UpdatedDate] FROM Employee WHERE EmployeeKey IN (@Param0, @Param1, @Param2, @Param3, @Param4);
@Param0: 11
@Param1: 12
@Param2: 13
@Param3: 14
@Param4: 15

If I batch it with another command, I only get 1 row back. (EmployeeKey 11 in this case.)

SELECT [EmployeeKey], [FirstName], [MiddleName], [LastName], [Title], [ManagerKey], [CreatedDate], [UpdatedDate] FROM Employee WHERE EmployeeKey IN (@Param0, @Param1, @Param2, @Param3, @Param4);
DELETE FROM Employee WHERE EmployeeKey IN (@Param0, @Param1, @Param2, @Param3, @Param4);
@Param0: 11
@Param1: 12
@Param2: 13
@Param3: 14
@Param4: 15

This only happens with Microsoft.Data.Sqlite. If I run the same batch with System.Data.SQLite I get back the expected number of rows.

@Grauenwolf

This comment has been minimized.

Copy link
Author

commented Sep 7, 2017

Here's my package list in case its relevant.

<?xml version="1.0" encoding="utf-8"?>
<packages>
  <package id="Microsoft.Bcl" version="1.1.10" targetFramework="net461" />
  <package id="Microsoft.Bcl.Async" version="1.0.168" targetFramework="net461" />
  <package id="Microsoft.Bcl.Build" version="1.0.21" targetFramework="net461" />
  <package id="Microsoft.Data.Sqlite" version="2.0.0" targetFramework="net461" />
  <package id="Microsoft.Data.Sqlite.Core" version="2.0.0" targetFramework="net461" />
  <package id="Microsoft.NETCore.Platforms" version="1.0.1" targetFramework="net461" />
  <package id="Microsoft.Win32.Primitives" version="4.0.1" targetFramework="net461" />
  <package id="MSTest.TestAdapter" version="1.1.18" targetFramework="net461" />
  <package id="MSTest.TestFramework" version="1.1.18" targetFramework="net461" />
  <package id="Nito.AsyncEx" version="4.0.1" targetFramework="net461" />
  <package id="SQLitePCLRaw.bundle_green" version="1.1.7" targetFramework="net461" />
  <package id="SQLitePCLRaw.core" version="1.1.8" targetFramework="net461" />
  <package id="SQLitePCLRaw.lib.e_sqlite3.linux" version="1.1.7" targetFramework="net461" />
  <package id="SQLitePCLRaw.lib.e_sqlite3.osx" version="1.1.7" targetFramework="net461" />
  <package id="SQLitePCLRaw.lib.e_sqlite3.v110_xp" version="1.1.7" targetFramework="net461" />
  <package id="SQLitePCLRaw.provider.e_sqlite3.net45" version="1.1.7" targetFramework="net461" />
  <package id="System.AppContext" version="4.1.0" targetFramework="net461" />
  <package id="System.Collections" version="4.0.11" targetFramework="net461" />
  <package id="System.Collections.Concurrent" version="4.0.12" targetFramework="net461" />
  <package id="System.Collections.Immutable" version="1.3.1" targetFramework="net461" />
  <package id="System.Console" version="4.0.0" targetFramework="net461" />
  <package id="System.Diagnostics.Debug" version="4.0.11" targetFramework="net461" />
  <package id="System.Diagnostics.DiagnosticSource" version="4.0.0" targetFramework="net461" />
  <package id="System.Diagnostics.Tools" version="4.0.1" targetFramework="net461" />
  <package id="System.Diagnostics.Tracing" version="4.1.0" targetFramework="net461" />
  <package id="System.Globalization" version="4.0.11" targetFramework="net461" />
  <package id="System.Globalization.Calendars" version="4.0.1" targetFramework="net461" />
  <package id="System.IO" version="4.1.0" targetFramework="net461" />
  <package id="System.IO.Compression" version="4.1.0" targetFramework="net461" />
  <package id="System.IO.Compression.ZipFile" version="4.0.1" targetFramework="net461" />
  <package id="System.IO.FileSystem" version="4.0.1" targetFramework="net461" />
  <package id="System.IO.FileSystem.Primitives" version="4.0.1" targetFramework="net461" />
  <package id="System.Linq" version="4.1.0" targetFramework="net461" />
  <package id="System.Linq.Expressions" version="4.1.0" targetFramework="net461" />
  <package id="System.Net.Http" version="4.1.0" targetFramework="net461" />
  <package id="System.Net.Primitives" version="4.0.11" targetFramework="net461" />
  <package id="System.Net.Sockets" version="4.1.0" targetFramework="net461" />
  <package id="System.ObjectModel" version="4.0.12" targetFramework="net461" />
  <package id="System.Reflection" version="4.1.0" targetFramework="net461" />
  <package id="System.Reflection.Extensions" version="4.0.1" targetFramework="net461" />
  <package id="System.Reflection.Primitives" version="4.0.1" targetFramework="net461" />
  <package id="System.Resources.ResourceManager" version="4.0.1" targetFramework="net461" />
  <package id="System.Runtime" version="4.1.0" targetFramework="net461" />
  <package id="System.Runtime.Extensions" version="4.1.0" targetFramework="net461" />
  <package id="System.Runtime.Handles" version="4.0.1" targetFramework="net461" />
  <package id="System.Runtime.InteropServices" version="4.1.0" targetFramework="net461" />
  <package id="System.Runtime.InteropServices.RuntimeInformation" version="4.0.0" targetFramework="net461" />
  <package id="System.Runtime.Numerics" version="4.0.1" targetFramework="net461" />
  <package id="System.Security.Cryptography.Algorithms" version="4.2.0" targetFramework="net461" />
  <package id="System.Security.Cryptography.Encoding" version="4.0.0" targetFramework="net461" />
  <package id="System.Security.Cryptography.Primitives" version="4.0.0" targetFramework="net461" />
  <package id="System.Security.Cryptography.X509Certificates" version="4.1.0" targetFramework="net461" />
  <package id="System.Text.Encoding" version="4.0.11" targetFramework="net461" />
  <package id="System.Text.Encoding.Extensions" version="4.0.11" targetFramework="net461" />
  <package id="System.Text.RegularExpressions" version="4.1.0" targetFramework="net461" />
  <package id="System.Threading" version="4.0.11" targetFramework="net461" />
  <package id="System.Threading.Tasks" version="4.0.11" targetFramework="net461" />
  <package id="System.Threading.Timer" version="4.0.1" targetFramework="net461" />
  <package id="System.ValueTuple" version="4.4.0" targetFramework="net461" />
  <package id="System.Xml.ReaderWriter" version="4.0.11" targetFramework="net461" />
  <package id="System.Xml.XDocument" version="4.0.11" targetFramework="net461" />
  <package id="Tortuga.Anchor" version="1.2.6283.989" targetFramework="net461" />
  <package id="xunit.abstractions" version="2.0.1" targetFramework="net461" />
  <package id="xunit.core" version="2.2.0" targetFramework="net461" />
  <package id="xunit.extensibility.core" version="2.2.0" targetFramework="net461" />
  <package id="xunit.extensibility.execution" version="2.2.0" targetFramework="net461" />
  <package id="xunit.runner.visualstudio" version="2.2.0" targetFramework="net461" developmentDependency="true" />
</packages>
@bricelam

This comment has been minimized.

Copy link
Member

commented Sep 7, 2017

Weird. I wonder if we're hitting undefined behavior in sqlite3_changes()...

Can you share your full repro code? This is more likely an issue with how the parameters are being bound. (e.g. all but one of the paramaters could be NULL)

@bricelam

This comment has been minimized.

Copy link
Member

commented Sep 7, 2017

I see what's going on:

  1. We execute the first statement and get a reader
  2. We execute the second statement
  3. We iterate the results of the reader from step 1, but the remaining rows have been deleted
@bricelam

This comment has been minimized.

Copy link
Member

commented Sep 7, 2017

Shouldn't SQLite's serialized isolation prevent this? It seems pretty bad that open readers can be affected by subsequent statements...

@bricelam

This comment has been minimized.

Copy link
Member

commented Sep 7, 2017

Getting SQLITE_BUSY from the DELETE statement seems more appropriate.

@bricelam

This comment has been minimized.

Copy link
Member

commented Sep 7, 2017

Confirmed. You'll get the same behavior with the following code.

var selectCommand = connection.CreateCommand();
selectCommand.CommandText = "SELECT * FROM Employee;";
var reader = selectCommand.ExecuteReader();

var deleteCommand = connection.CreateCommand();
deleteCommand.CommandText = "DELETE FROM Employee;";
deleteCommand.ExecuteNonQuery();

var count = 0;
while (reader.Read())
{
    count++;
}

// count == 1
@bricelam

This comment has been minimized.

Copy link
Member

commented Sep 7, 2017

We have two options if we want to change this behavior when executing in a batch:

  1. Buffer the results (read all the results into memory before executing DELETE)
  2. Make SqliteDataReader.ExecuteReader() only execute the first statement and require SqliteDataReader.NextResult() to be called before the DELETE statement is executed.
@bricelam

This comment has been minimized.

Copy link
Member

commented Sep 7, 2017

It looks like System.Data.SQLite does option 2.

@Grauenwolf

This comment has been minimized.

Copy link
Author

commented Sep 8, 2017

In the non-buffering case, wouldn't it be enough to exhaust the DataReader by calling .Read() until it returns false? I don't think it's necessary to actually call .NextResult unless you intend to discard the first result set.

@bricelam

This comment has been minimized.

Copy link
Member

commented Sep 8, 2017

Read() goes to the next row. NextResult() goes to the next statement.

If you don't call NextResult(), the remaining statements would be executed when you dispose the reader. (This is how System.Data.SQLite works.)

I was very much against this implementation during the creation of Microsoft.Data.Sqlite since it causes exceptions to be thrown at very unexpected times--during NextResult() and Dispose(). With the current implementation all execution-related exceptions are throw during ExecuteReader(). I didn't, however, anticipate the awkward behavior described in this issue.

@Grauenwolf

This comment has been minimized.

Copy link
Author

commented Sep 8, 2017

If you don't call NextResult(), the remaining statements would be executed when you dispose the reader. (This is how System.Data.SQLite works.)

Ah, that must be what my code is relying on.

What's the behavior of say SQL Server?

@bricelam

This comment has been minimized.

Copy link
Member

commented Sep 8, 2017

In SQL Server, I don't think open readers are affected by subsequent statements. The reader's results are isolated on the server; it marks the rows as deleted, but keeps the data around until the reader is closed.

@bricelam

This comment has been minimized.

Copy link
Member

commented Sep 8, 2017

I'll investigate the behavior of SqlClient more and report back.

@bricelam bricelam self-assigned this Sep 8, 2017

@bricelam bricelam added this to the 2.1.0 milestone Sep 8, 2017

@bricelam

This comment has been minimized.

Copy link
Member

commented Jan 26, 2018

SQL Server behavior:

  • DELETE statement is applied when ExecuteReader returns
  • All the pre-DELETE rows are returned when iterating over the reader.

This confirms my speculation--the server keeps the rows around for open readers.

@bricelam bricelam removed this from the 2.1.0 milestone Jan 26, 2018

@bricelam bricelam removed the propose-punt label Jan 26, 2018

@bricelam

This comment has been minimized.

Copy link
Member

commented Jan 26, 2018

So, we have three options:

  • Keep everything as is (let SQLite's behavior leak into our batching implementation)
  • Delay the execution of DELETE until NextResult() (or Dispose()/the finalizer) is called
  • Buffer the results of SELECT on the client before executing DELETE
@Grauenwolf

This comment has been minimized.

Copy link
Author

commented Jan 26, 2018

#1 seems like a very bad idea. It's definitely not what people are going to expect and doesn't match the behavior on .NET Framework with the non-MS drivers.

@bricelam

This comment has been minimized.

Copy link
Member

commented Jan 27, 2018

When I first thought about this during the original implementation, I avoided option 2 like the plague. Even though it's the approach System.Data.SQLite took, it felt very wrong to finish executing the statements on Dispose() and swallow exceptions.

If we decide we want it to behave like SqlClient, I think we can do it without too big of a perf/memory hit if we wait until we encounter a non-readonly statement. Only then would we buffer the previous result sets. This allows us to continue streaming when the batch contains only SELECT statements. Also, we already buffer the first row, so a typical case of INSERT...; SELECT last_insert_rowid(); INSERT...; SELECT...; shouldn't see any difference.

@bricelam

This comment has been minimized.

Copy link
Member

commented Jan 27, 2018

P.S. (In case anyone has the same thought) executing the batch within a transaction does not change SQLite's behavior.

@bricelam bricelam added this to the Backlog milestone Jan 29, 2018

@bricelam

This comment has been minimized.

Copy link
Member

commented Jan 29, 2018

Triage: We decided that we should fix this by delaying the execution of DELETE until NextResult() is called. This has subtle behavior changes, but for most cases it won't affect applications.

@bricelam bricelam removed this from the Backlog milestone Jan 29, 2018

@bricelam bricelam added this to the Breaking changes milestone May 2, 2018

@ajcvickers ajcvickers modified the milestones: Breaking changes, 3.0.0 Jun 5, 2018

@ajcvickers ajcvickers transferred this issue from aspnet/Microsoft.Data.Sqlite Oct 31, 2018

@ajcvickers ajcvickers added this to the 3.0.0 milestone Oct 31, 2018

@ajcvickers ajcvickers changed the title Results affected by subsequent statements when batching Microsoft.Data.Sqlite: Results affected by subsequent statements when batching Oct 31, 2018

bricelam added a commit to bricelam/EFCore that referenced this issue Mar 9, 2019
Microsoft.Data.Sqlite: Delay statement execution until NextResult
TODO
- Test RecordsAffected
- Does execution stop when a statement throws?
- Are there leaks when a statement throws?
- Review SqliteDataReader finalizer

Fixes aspnet#13830
bricelam added a commit to bricelam/EFCore that referenced this issue Mar 11, 2019
Microsoft.Data.Sqlite: Delay statement execution until NextResult
TODO
- Does execution stop when a statement throws?
- Are there leaks when a statement throws?
- Review SqliteDataReader finalizer

Fixes aspnet#13830
bricelam added a commit to bricelam/EFCore that referenced this issue Mar 12, 2019
Microsoft.Data.Sqlite: Delay statement execution until NextResult
TODO
- Does execution stop when a statement throws?
- Are there leaks when a statement throws?
- Review SqliteDataReader finalizer

Fixes aspnet#13830
bricelam added a commit to bricelam/EFCore that referenced this issue Mar 12, 2019
bricelam added a commit to bricelam/EFCore that referenced this issue Mar 12, 2019
bricelam added a commit to bricelam/EFCore that referenced this issue Mar 13, 2019
bricelam added a commit to bricelam/EFCore that referenced this issue Mar 15, 2019
bricelam added a commit that referenced this issue Mar 18, 2019

@ajcvickers ajcvickers modified the milestones: 3.0.0, 3.0.0-preview4 Apr 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.