ericsink / SQLitePCL.raw

A Portable Class Library (PCL) for low-level (raw) access to SQLite
Apache License 2.0
513 stars 106 forks source link

sqlite3_trace_v2 requires IntPtr to sqlite3 and sqlite3_stmt #272

Open bricelam opened 5 years ago

bricelam commented 5 years ago

As far as I can tell, there is no way to convert an IntPtr to a sqlite3 or sqlite3_stmt. This limits the usefulness of sqlite3_trace_v2:

sqlite3_trace_v2(
    db,
    SQLITE_TRACE_STMT | SQLITE_TRACE_PROFILE | SQLITE_TRACE_ROW | SQLITE_TRACE_CLOSE,
    (d, m, p, x) =>
    {
        switch (d)
        {
            case SQLITE_TRACE_STMT:
                // No way to convert IntPtr p to sqlite3_stmt
                var sql = Marshal.PtrToStringUTF8(x);
                break;

            case SQLITE_TRACE_PROFILE:
                // No way to convert IntPtr p to sqlite3_stmt
                var nanoseconds = Marshal.PtrToStructure<long>(x);
                break;

            case SQLITE_TRACE_ROW:
                // No way to convert IntPtr p to sqlite3_stmt
                break;

            case SQLITE_TRACE_CLOSE:
                // No way to convert IntPtr p to sqlite3
                break;
        }        

        return 0;
    },
    null);
ericsink commented 5 years ago

Hmm. Right you are. I didn't take this far enough to experience that problem.

My initial thought is that I can expose sqlite3_trace_v2() with a different signature, one that takes 4 typed delegates instead of one that requires unsafe casting.

ericsink commented 5 years ago

But that'll bump into the same problem that we have with sqlite3_next_stmt(). We don't want to re-wrap the IntPtr as a sqlite3_stmt, this ending up with two sqlite3_stmt handles that both refer to the same underlying pointer. We want to return the exact same sqlite3_stmt instance we already created.

And doing that for sqlite3_next_stmt() involved keeping a dictionary to map them. And because sqlite3_next_stmt() isn't commonly used, I added code to disable that feature to save performance.

So this would bring that dictionary back. And one for sqlite3 db handles as well.

bricelam commented 5 years ago

I like the idea of a higher-level abstraction. You can't really combine flags without later using a switch statement to separate them back out again anyway. Creating an overload for each value sounds more sensible.

No rush here; I was just updating a sample that happened to use sqlite3_trace.

This issue has a lot of overlap with #269.

ericsink commented 5 years ago

A proper implementation of sqlite3_trace_v2() is a can of worms, and it doesn't seem like a showstopper for v2. My current plan is to remove the half-done feature and revisit it later.