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

SQLiteConn: Add 'Raw' method to give access to underlying sqlite3 con… #785

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gwd
Copy link

@gwd gwd commented Feb 9, 2020

…text structure

This is necessary, for instance, to allow users of the package to make
their own CGo callbacks, and have the underlying sqlite3 library call
those C functions directly.

Note that the intention from the sqlite3 library seems to be that
there should be a measure of binary compatibility between versions:
even extensions compiled against an older different version of sqlite3
should be able to call sqlite3_create_function with no problems. So
this should work even if users of the package have a slightly
different version of the sqlite3.h header.

Note that the code itself was written by @rittneje. I tested it and
wrote the comments.

Signed-off-by: George Dunlap [email protected]

v2:
Just copy @rittneje's code exactly:

  • Grab the lock while the function is running
  • Replace the error code with our own if it's of type ErrNo

@gwd
Copy link
Author

gwd commented Feb 9, 2020

If you give me some guidance, I could write a test case for adding a Cgo callback. It seems like it might be easier to manage in its own file; sqlite_raw_test.go maybe?

@rittneje
Copy link
Collaborator

rittneje commented Feb 9, 2020

You cannot do cgo directly in a test file unfortunately. You’ll need to make an auxiliary package that, for example, calls Raw and registers a custom function. Then your unit test can leverage that auxiliary package, and then run a query to verify the function got registered properly. To avoid an import cycle, your test will need to be in the sqlite3_test package (but should still be in the go-sqlite3 directory).

@gwd
Copy link
Author

gwd commented Feb 9, 2020

@rittneje Ah yes, I remember that now. Well, I think making a new package is a bit beyond my enthusiasm at the moment. :-) I'm not sure it will make the coverage bot happier anyway.

@coveralls
Copy link

coveralls commented Feb 9, 2020

Coverage Status

Coverage increased (+0.08%) to 53.48% when pulling b062b0f on gwd:out/raw-context/v2 into 4c2df3c on mattn:master.

@rittneje
Copy link
Collaborator

rittneje commented Feb 9, 2020

I think writing the test is useful, especially because it serves as a convenient example for anyone wishing to do what you are doing. Also, it should count towards coverage as long as you do something like this.

github.com/mattn/go-sqlite3/internal/sqlite3test/raw.go

package sqlite3test

/*
#include <stdlib.h>
#include <sqlite3.h>

static void one(sqlite3_context* ctx, int n, sqlite3_value** vals) {
    sqlite3_result_int(ctx, 1);
}

static inline int _create_function(sqlite3* c) {
    return sqlite3_create_function(c, "one", 0, SQLITE_UTF8|SQLITE_DETERMINISTIC, NULL, one, NULL, NULL);
}
*/
import "C"
import (
    "unsafe"
    sqlite3 "github.com/mattn/go-sqlite3"
)

func RegisterFunction(conn *sqlite3.SQLiteConn) error {
    return conn.Raw(func(raw unsafe.Pointer) error) error {
        rawConn := (*C.sqlite3)(raw)
        return C._create_function(rawConn)
    }
}

github.com/mattn/go-sqlite3/raw_test.go

package sqlite3_test

import (
    "context"
    "database/sql/driver"
    sqlite3 "github.com/mattn/go-sqlite3"
    "github.com/mattn/go-sqlite3/internal/sqlite3test"
)

type connector struct{
    filename string
    d *sqlite3.Driver
}

func (rc rawConnector) Connect(context.Context) (driver.Conn, error) {
    return rc.d.Open(rc.filename)
}

func (rc rawConnector) Driver() driver.Driver {
    return rc.d
}

func TestRaw(t *testing.T) {
    connector := rawConnector{
        filename: "...",
        d:  &sqlite3.Driver{
            ConnectHook(func(c *sqlite3.Conn) error {
                return sqlite3test.RegisterFunction(c)
            },
        },
    }

    db := sql.OpenDB(connector)
    defer db.Close()

    if err := db.Ping(); err != nil {
        t.Fatal(err)
    }

    var result int
    if err := db.QueryRow(`SELECT one()`).Scan(&result); err != nil {
        t.Fatal(err)
    }

    if result != 1 {
        t.Errorf("expected custom one() function to return 1, but got %d", result)
    }
}

Note that the raw_test.go file is in the go-sqlite3 directory, but the sqlite3_test package. This allows it to contribute the the coverage results, and not cause an important cycle.

@gwd
Copy link
Author

gwd commented Jul 22, 2020

@rittneje OK, I've finally gotten around to taking your advice regarding the testing. Hopefully this gets a green check for coverage from Travis.

@gwd
Copy link
Author

gwd commented Jul 22, 2020

OK, v4 to fix the build in Golang 1.9, which doesn't have sql.OpenDB()

@gwd
Copy link
Author

gwd commented Aug 3, 2020

@mattn Any update on this PR? It's a pretty self-contained change, which I've been using for several months now. It's had some really good input from @rittneje, and has testing coverage.

@gwd
Copy link
Author

gwd commented Sep 15, 2020

@mattn @rittneje Any more comments?

This has a pretty massive impact on my query, which makes heavy use of a custom function. Below are microbenchmark results; C-12 is the direct C callback implemented using the functionality this PR provides. As you can see, it's over 15x faster than the golang callbacks.

BenchmarkDcalc/Go-12                      809586	      1533 ns/op
BenchmarkDcalc/GoCompat-12         	  771516	      1579 ns/op
BenchmarkDcalc/C-12                	12472881	       105 ns/op

@codecov-commenter
Copy link

codecov-commenter commented Sep 7, 2022

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (00b02e0) 46.72% compared to head (1e5b749) 46.70%.

Files Patch % Lines
sqlite3.go 42.85% 3 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #785      +/-   ##
==========================================
- Coverage   46.72%   46.70%   -0.02%     
==========================================
  Files          12       12              
  Lines        1526     1533       +7     
==========================================
+ Hits          713      716       +3     
- Misses        671      674       +3     
- Partials      142      143       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gwd
Copy link
Author

gwd commented Sep 7, 2022

@rittneje Hey, it's been like 2 and a half years now, and I'm still rebasing my one little patch. What needs to be done to get this merged? 😁

@rittneje
Copy link
Collaborator

@gwd This PR is on my to-do list and I will take a look when I have time.

…text structure

This is necessary, for instance, to allow users of the package to make
their own CGo callbacks, and have the underlying sqlite3 library call
those C functions directly.

Note that the intention from the sqlite3 library seems to be that
there should be a measure of binary compatibility between versions:
even extensions compiled against an older different version of sqlite3
should be able to call sqlite3_create_function with no problems. So
this should work even if users of the package have a slightly
different version of the sqlite3.h header.

Note that the code itself was written by @rittneje.  I tested it and
wrote the comments.

Signed-off-by: George Dunlap <[email protected]>

v2:
Just copy @rittneje's code exactly:
- Grab the lock while the function is running
- Replace the error code with our own if it's of type ErrNo
Note that you can't call cgo from within a test file; so in order to
test the functionality, create new package, .../internal/sqlite3test,
to define the callback.  Then in the main sqlite3 package, add a test
file which includes this package and calls the callback.

As with the previous commit, the idea and the basic code was written
by @rittneje; I massaged it so that it compiled and passed with Go
1.9, and wrote this commit message and the comments.

NB that Golang 1.9 doesn't have sql.OpenDB, so we have to register a
new database driver.

Signed-off-by: George Dunlap <[email protected]>

v4:
- Refactor to get rid of sql.OpenDB call, which only appeared in Go
  1.10

v3:
- New
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants