Add Serialize and Deserialize support

[otoolep]

No description provided.

[otoolep]

It's not clear to me why the build is failing because:

• sqlite3_serialize() is available in the C source.
• SQLITE_OMIT_DESERIALIZE is not defined anywhere that I can see so the C function should be available.

What am I missing?

[codecov-commenter]

Codecov Report

Base: 46.09% // Head: 46.80% // Increases project coverage by +0.70% 🎉

Coverage data is based on head (9ec6bc6) compared to base (7476442).
Patch coverage: 82.85% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1089      +/-   ##
==========================================
+ Coverage   46.09%   46.80%   +0.70%     
==========================================
  Files          11       12       +1     
  Lines        1499     1534      +35     
==========================================
+ Hits          691      718      +27     
- Misses        669      673       +4     
- Partials      139      143       +4     

Impacted Files	Coverage Δ
sqlite3_opt_serialize.go	82.85% <82.85%> (ø)
sqlite3.go	52.64% <0.00%> (-0.23%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[rittneje]

@otoolep Prior to v3.36.0, sqlite3_serialize and sqlite3_deserialize were opt in via -DSQLITE_ENABLE_DESERIALIZE. This is why the libsqlite3 tests are failing. Also now they are opt out via -DSQLITE_OMIT_DESERIALIZE, which we should support.

We will either need a Go build tag to enable these new methods, or a tag to disable them. Personally, my preference would be to make it opt out so things "just work" by default, but technically that could be a breaking change for anyone currently using the libsqlite3 tag. @mattn what do you think?

[mattn]

Personally, my preference would be to make it opt out so things "just work" by default

Do you mean that the functions return like ErrNotImplemented?

[rittneje]

@mattn Historically this library has been inconsistent between having alternate version of the Go methods that return errors, or simply not defining the Go methods at all. I don't care which one we do here. The important part is that if you opt out (or don't opt in, depending on our approach), we never attempt to reference the C functions.

[otoolep]

I can add a build tag for either option (opt-in or opt-out), I forgot about the libsqlite3 tests. The lack of this patch is the only thing forcing rqlite to use a fork of this repo.

[mattn]

type Serializable interface {
  Serialize(schema string) []byte
}

if ser, ok := conn.(Serializable); ok {
  b = ser.Serialize(name)
  // blah
} else {
  // not implemented
}

We can do check whether the SQLiteConn has methods like above, I think. @rittneje what do you think?

[rittneje]

@mattn The underlying problem is we cannot reference the sqlite3_serialize or sqlite3_deserialize C functions if SQLite itself was compiled without them. For the statically compiled version this is simple because we control the build options, but for someone linking against their system version we have no way of knowing unless they tell us. These are the choices I can see:

1. We make our references to those two C functions opt out. This means anyone using libsqlite3 today could be broken (if their system version was compiled without support) and will have to add an additional Go build tag so we don't attempt to reference those functions.
2. We make our references to those C functions opt in. This preserves backwards compatibility for anyone using libsqlite3 today, but in exchange anyone that wants to use these new Go methods is required to pass an additional tag to go build, which is annoying.
3. We always exclude these new methods if libsqlite3 is defined.
4. We make these new methods opt in if you are using libsqlite3. Otherwise they are always defined.
5. We make these new methods opt in if you are using libsqlite3, and opt out if you aren't.

Us dynamically checking what methods SQLiteConn has does not help, since we are the ones that have to define those very methods in the first place.

[mattn]

Thanks your explaining. As you mentioned above, let's add sqlite_omit_deserialize.

[rittneje]

To be clear, you are saying to do option 1 from my list?

[mattn]

I have thought about this for a little while and I expect the following.

1. opt-in if using libsqlite3, default is undefined
2. always enabled if libsqlite3 is not used

[mattn]

I assume that people who use libsqlite3 are expecting minimalism and they does not want these functions. I guess.

[rittneje]

I assume that people who use libsqlite3 are expecting minimalism and they does not want these functions. I guess.

Depends on their use case. They could also just want consistency across applications written in multiple languages.

[otoolep]

@rittneje @mattn -- does this look like what you had in mind?

I also don't understand the "dockerfile" CI failure. Can you help?

[rittneje]

I also don't understand the "dockerfile" CI failure. Can you help?

Seems that #1085 changed the output of the sample program, but did not change the expectation in the GitHub action, so it fails.

go-sqlite3/.github/workflows/docker.yaml

Line 22 in 7476442

docker run simple | grep 99\ こんにちわ世界099

That said, I don't know why the output is checked in both the Dockerfile itself as well as the GitHub action. @mattn Can one of them be removed?

[otoolep]

I think we're good now. Let me know there are more required changes.

[otoolep]

What do you guys think, ready to merge?

[otoolep]

I think we're good to merge this?

[otoolep]

@rittneje -- I think we're good to merge this? Anything else need doing? The test has been updated, and all tests pass.

With this merged I can return to using this repo, and dump my fork.

[otoolep]

Thank you! Looking forward to moving back to this repo, and away from my fork.

[mattn]

@otoolep Thank you.