cmeeren / Facil

Facil generates F# data access source code from SQL queries and stored procedures. Optimized for developer happiness.
MIT License
140 stars 7 forks source link

Error loading temp table data within a sql transaction #32

Closed costa100 closed 2 years ago

costa100 commented 2 years ago

Hi again,

Sorry, I wish that I didn't have to create these issues and that everything worked fine.

What I am trying to do? Within a sql transaction:

The problem that I encountered is when I save the data to the temp tables:

System.InvalidOperationException: ExecuteNonQuery requires the command to have a transaction when the connection assigned to the command is in a pending local transaction.  The Transaction property of the command has not been initialized.
 at Microsoft.Data.SqlClient.SqlCommand.ValidateCommand(Boolean isAsync, String method)
   at Microsoft.Data.SqlClient.SqlCommand.InternalExecuteNonQuery(TaskCompletionSource`1 completion, Boolean sendToPipe, Int32 timeout, Boolean& usedCache, Boolean asyncWrite, Boolean inRetry, String methodName)
   at Microsoft.Data.SqlClient.SqlCommand.ExecuteNonQuery()
   at Facil.Runtime.CSharp.GeneratedCodeUtils.LoadTempTables(SqlConnection conn, IEnumerable`1 tempTableData) in C:\projects\facil\src\Facil.Runtime.CSharp\GeneratedCodeUtils.cs:line 48
   at Facil.Runtime.CSharp.GeneratedCodeUtils.ExecuteQueryEager[T](SqlConnection existingConn, String connStr, Action`1 configureNewConn, Action`1 configureCmd, Action`1 initOrdinals, Func`2 getItem, IEnumerable`1 tempTableData) in C:\projects\fac
il\src\Facil.Runtime.CSharp\GeneratedCodeUtils.cs:line 150

Basically this code ends up being called and it blows up when it tries to execute cmd.ExecuteNonQuery()

        private static void LoadTempTables(SqlConnection conn, IEnumerable<TempTableData> tempTableData)
        {
            foreach (var data in tempTableData)
            {
                using var cmd = conn.CreateCommand();
                // Note: If there is ever a need for letting users configure the command,
                // do not use the configureCmd parameter passed to methods on this class,
                // which also sets any parameters.
                cmd.CommandText = data.Definition;
                cmd.ExecuteNonQuery();

                using var bulkCopy = new SqlBulkCopy(conn) { DestinationTableName = data.DestinationTableName };
                data.ConfigureBulkCopy(bulkCopy);
                var reader = new TempTableLoader(data.NumFields, data.Data);
                bulkCopy.WriteToServer(reader);
            }
        }

Any idea on how to workaround this? I cannot move the processing of the data and the saving of the processed data into the temp tables outside the transaction.

Thanks

costa100 commented 2 years ago

Ok, I ended up writing my own bulk copy code based on the Facil code. BulkCopyTempDataLoader is based on the TempDataLoader. I would have used it by it is marked as internal.

module ProcessingUtils

open System.Data
open Microsoft.Data.SqlClient
open FSharp.Data
open Facil.Runtime.CSharp

(*

        private static void LoadTempTables(SqlConnection conn, IEnumerable<TempTableData> tempTableData)
        {
            foreach (var data in tempTableData)
            {
                using var cmd = conn.CreateCommand();
                // Note: If there is ever a need for letting users configure the command,
                // do not use the configureCmd parameter passed to methods on this class,
                // which also sets any parameters.
                cmd.CommandText = data.Definition;
                cmd.ExecuteNonQuery();

                using var bulkCopy = new SqlBulkCopy(conn) { DestinationTableName = data.DestinationTableName };
                data.ConfigureBulkCopy(bulkCopy);
                var reader = new TempTableLoader(data.NumFields, data.Data);
                bulkCopy.WriteToServer(reader);
            }
        }
*)

type BulkCopyTempDataLoader(fieldCount: int, data: seq<obj array>) =

      let enumerator = data.GetEnumerator()

      interface IDataReader with
        member x.Read() = enumerator.MoveNext()
        member x.FieldCount with get() = fieldCount
        member x.GetValue i = enumerator.Current.[i]

        member x.Close() = failwith "not implemented"
        member x.Dispose() = ()
        member x.GetBoolean i = failwith "not implemented"
        member x.GetByte i = failwith "not implemented"
        member x.GetBytes(i, fieldOffset, buffer, bufferoffset, length) = failwith "not implemented"
        member x.GetChar i = failwith "not implemented"
        member x.GetChars(i, fieldOffset, buffer, bufferoffset, length) = failwith "not implemented"
        member x.GetData i = failwith "not implemented"
        member x.GetDataTypeName i = failwith "not implemented"
        member x.GetDateTime i = failwith "not implemented"
        member x.GetDecimal i = failwith "not implemented"
        member x.GetDouble i = failwith "not implemented"
        member x.GetFieldType i = failwith "not implemented"
        member x.GetFloat i = failwith "not implemented"
        member x.GetGuid i = failwith "not implemented"
        member x.GetInt16 i = failwith "not implemented"
        member x.GetInt32 i = failwith "not implemented"
        member x.GetInt64 i = failwith "not implemented"
        member x.GetName i = failwith "not implemented"
        member x.GetOrdinal name = failwith "not implemented"
        member x.GetSchemaTable() = failwith "not implemented"
        member x.GetString i = failwith "not implemented"

        member x.GetValues values = failwith "not implemented"
        member x.IsDBNull i = failwith "not implemented"
        member x.NextResult() = failwith "not implemented"
        member x.Depth with get() = 0
        member x.IsClosed with get() = failwith "not implemented"
        member x.RecordsAffected with get() = failwith "not implemented"
        member x.Item
            with get (name: string) : obj = failwith "not implemented"
        member x.Item
            with get (i: int) : obj = failwith "not implemented"

let bulkCopyData (cn: SqlConnection) (tran: SqlTransaction) (configCmd: SqlCommand -> unit) (tempTableData: #seq<TempTableData>) =
  tempTableData
    |> Seq.iter (fun data ->

      use cmd = cn.CreateCommand()
      configCmd cmd

      cmd.CommandText <- data.Definition
      cmd.ExecuteNonQuery() |> ignore

      use bulkCopy = new SqlBulkCopy (cn, SqlBulkCopyOptions.Default, tran)
      bulkCopy.DestinationTableName <- data.DestinationTableName
      bulkCopy.BatchSize <- 10000

      use reader = new BulkCopyTempDataLoader(data.NumFields, data.Data)
      bulkCopy.WriteToServer(reader)

    )

Here is an example of use:

    Scripts.SomeTempTable
      .WithConnection(someCn)
      .ConfigureCommand(jc.defaultCmdConfig(tran))
     // This extracts one dataset from the outputRows collection
      .CreateTempTableData(outputRows |> Seq.map(fun it -> it.jobTemplate) |> Seq.choose id)
    |> bulkCopyData someCn tran (jc.defaultCmdConfig(tran)) 

where jc is an object of a class with this method:

  member this.defaultCmdConfig (tran: SqlTransaction) = 
    fun (cmd:SqlCommand) -> 
        cmd.CommandTimeout <- 0
        if tran <> null then
          cmd.Transaction <- tran

Any other ideas? If not, you can close this. Also if you want to use the code above, you can go ahead and do it - it is based on your code after all

Thanks

cmeeren commented 2 years ago

Sorry, I wish that I didn't have to create these issues and that everything worked fine.

No worries, just means that either there are bugs, or you're using Facil in ways I have not intended and which may or may not fall within the project's scope. Let's try to figure that out.

What I am trying to do? Within a sql transaction:

  • Get rows to process <- this has to happen within the transaction
  • Process rows and gather data in memory
  • Save data in temp tables
  • Call stored procedure to merge the data from the temp tables into the real tables

Can't all of this be done in a single SQL script (which, as the last step, invokes the desired procedure)?

If not, I need a minimal repro so that I see exactly what you are trying to do. I realize the DB part can be hard in a minimal repro, but if you at least can post a project containing the generated code, and your code that invokes it, that's very helpful. Even more so if you have a separate script to generate the tables, so I can test myself. Keep in mind I'm asking for a minimal repro, so remove anything that's not strictly required to reproduce. Extra code, tables, columns, etc.

costa100 commented 2 years ago

Can't all of this be done in a single SQL script (which, as the last step, invokes the desired procedure)?

I am doing data processing in F# because it is more productive and flexible when it comes to validating and transforming data.

I need a minimal repro so that I see exactly what you are trying to do.

https://github.com/costa100/TestFacilTransaction/tree/master/TestTempTable

I created a test case that reproduces the error that I experience in my real project. I hope this helps. I don't think the code should fail if I execute the operations inside a transaction.

costa100 commented 2 years ago

It would also be nice if Facil can support out-of-the-box inserting the data in the temp table and selecting the data from the temp table as two individual operations instead of supporting only insertion of data and selection as a single operation.

costa100 commented 2 years ago

I added one more test case related to the previous comment.

cmeeren commented 2 years ago

This should now work. After 2.2.0 is published, use the new WithConnection(conn, tran) overload to pass the transaction instead of using ConfigureCommand. (Yes, this means you also got #27. It's Christmas all over again! 🎅)

costa100 commented 2 years ago

Thank you again for making these changes.

One more question - is there any chance that you might make the bulk copy operation into a standalone action that won't invoke the retrieval of the data from the temp table? You could introduce a method similar to WithParameters, maybe called BulkCopy that would only shove the data into the temp table without retrieving it back.

Thanks!

cmeeren commented 2 years ago

Could you explain a use-case for that, that is hard/cumbersome/impossible with the current functionality?

costa100 commented 2 years ago

In my use case, I validate and extract data (coming from an external system) from a single table. The table has varchar(max) fields with json arrays which I have to save in different tables. I save the data in temp tables, but I don't need to select back from those tables. Then I call a stored procedure that moves the data from the temp tables into the final tables. The stored procedure does a bunch of merges and additional processing.

I got the whole thing working as you can see in this project: https://github.com/costa100/TestFacilTransaction/tree/master/TestTempTable: https://github.com/costa100/TestFacilTransaction/blob/master/TestTempTable/Tests.fs

The truth is I could use Facil to insert and then select data from the temp tables and use a condition where 1 = 0 to reduce the performance penalty from selecting the temp tables.

I thought it would be nice if your library offered the functionality out-of-the-box.

But maybe I am missing something.

cmeeren commented 2 years ago

I think I finally understand. It seems to me that this can be solved if you can specify temp tables for stored procedures just like you can with scripts. That should be fairly simple. I'll start working on it.

Just for the record, here are some possible workarounds:

cmeeren commented 2 years ago

v2.3.0 should be out shortly with support for temp tables in procedures, just like in scripts.

costa100 commented 2 years ago

Hi, thank you 🙏, I just rewrote the code to use temp tables with the stored procedure within a transaction and it works!

cmeeren commented 2 years ago

Happy to hear it!