Skip to content

Comments

Introducing the pure go driver for sqlite#557

Open
viscropst wants to merge 4 commits intok3s-io:masterfrom
viscropst:master
Open

Introducing the pure go driver for sqlite#557
viscropst wants to merge 4 commits intok3s-io:masterfrom
viscropst:master

Conversation

@viscropst
Copy link

@viscropst viscropst commented Dec 2, 2025

/close #556
and related to k0sproject/k0s#6730.

@viscropst viscropst marked this pull request as ready for review December 2, 2025 08:18
@viscropst viscropst requested a review from a team as a code owner December 2, 2025 08:18
Copy link
Member

@brandond brandond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test for this. You'll likely need to build a separate -nocgo image tag and/or binary, and then use that in the tests.

@viscropst
Copy link
Author

Please add a test for this. You'll likely need to build a separate -nocgo image tag and/or binary, and then use that in the tests.

Okay, I'll trying to do that.

@viscropst viscropst force-pushed the master branch 2 times, most recently from 460d771 to 192c703 Compare December 9, 2025 09:15
@viscropst viscropst requested a review from brandond December 9, 2025 09:16
@viscropst viscropst force-pushed the master branch 2 times, most recently from 26850ac to decd7c4 Compare December 9, 2025 10:06
@brandond
Copy link
Member

brandond commented Dec 9, 2025

I just realized that our Drone tests have not been running for a while - since CNCF decommissioned our dedicated hardware a month or so ago. I should have a PR merged shortly to move everything over to GHA; you can rebase on top of that and add tests to that.

@viscropst
Copy link
Author

viscropst commented Dec 10, 2025

I just realized that our Drone tests have not been running for a while - since CNCF decommissioned our dedicated hardware a month or so ago. I should have a PR merged shortly to move everything over to GHA; you can rebase on top of that and add tests to that.

Okay, I’ll rebase that later

@viscropst viscropst force-pushed the master branch 2 times, most recently from a9f2399 to ea7a215 Compare December 10, 2025 02:48
@brandond
Copy link
Member

CI looks good, lint has some complaints about the code though:

#13 88.13 pkg/drivers/sqlite/sqlite.go:44:2: var-naming: var dsnUrl should be dsnURL (revive)
#13 88.13 	dsnUrl, _ := url.Parse(dsn)
#13 88.13 	^
#13 88.13 pkg/drivers/sqlite/sqlite.go:56:1: File is not properly formatted (gofmt)
#13 88.13 	query.Set("_txlock", "immediate") 
#13 88.13 ^
#13 88.13 pkg/drivers/sqlite/sqlite_test.go:14:1: File is not properly formatted (gofmt)
#13 88.13 	dsn,_ := getDataSourceName("")
#13 88.13 ^

@viscropst
Copy link
Author

CI looks good, lint has some complaints about the code though:

#13 88.13 pkg/drivers/sqlite/sqlite.go:44:2: var-naming: var dsnUrl should be dsnURL (revive)
#13 88.13 	dsnUrl, _ := url.Parse(dsn)
#13 88.13 	^
#13 88.13 pkg/drivers/sqlite/sqlite.go:56:1: File is not properly formatted (gofmt)
#13 88.13 	query.Set("_txlock", "immediate") 
#13 88.13 ^
#13 88.13 pkg/drivers/sqlite/sqlite_test.go:14:1: File is not properly formatted (gofmt)
#13 88.13 	dsn,_ := getDataSourceName("")
#13 88.13 ^

Because of codespaces network latency, the codespaces could not formatting correctly. Now they are all fixed.

@brandond
Copy link
Member

Sorry, I pushed another round of changes to CI; would you mind rebasing again? I think we're very close to done with this.

@viscropst
Copy link
Author

Sorry, I pushed another round of changes to CI; would you mind rebasing again? I think we're very close to done with this.

Don't mind. It's normal for rapid CI migration from dedicated server to github CI.

@brandond
Copy link
Member

brandond commented Dec 11, 2025

I am planning to merge the NATS fix PR #549 before this one, so you will likely need to rebase one more time.

Would you like to add -nocgo variants to the release workflow, to produce pure-go images and binaries?

@viscropst
Copy link
Author

I am planning to merge the NATS fix PR #549 before this one, so you will likely need to rebase one more time.

Would you like to add -nocgo variants to the release workflow, to produce pure-go images and binaries?

For add -nocgo variants to release workflow, and rebae is yes. I want to add the test target in Makefile for convenient local testing, Can I add them in Makefile ?

@brandond
Copy link
Member

yeah go for it

@viscropst viscropst force-pushed the master branch 4 times, most recently from 6178544 to 8a1bdcf Compare December 22, 2025 02:06
@viscropst
Copy link
Author

seems sqlite tests are all still failing; I believe they were passing before you squashed. Please make sure nothing got mangled along the way.

I was running tests when I use the original dsn and got same result as same as original cgo driver.The parameters order of sqlite driver is not affected to cgo driver, because it's uses net/url.Values to get parameters and has its own order to processing the PRAGMAS. For nocgo drivers busy_time will pushed first and others will be sorted by raw index of _pragmas.

Oh hmm, something seems to have broken sqlite for both cgo and nocgo. Did something get mangled during the squash? Or is it related to how you converted the query params to pragmas?

Finally I found out the reason of compact conflict for both cgo and nocgo, because of the drivers parse the raw string to url queries to resolving the parameters, so they are not resolved properly.

@viscropst viscropst requested a review from brandond December 22, 2025 04:26
Copy link
Member

@brandond brandond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one more question about the checkpoint mode.

I will probably merge this after new years when I will be around to tag a release and fix CI if anything in the release pipeline does not work when tested.

func New(_ context.Context, _ *sync.WaitGroup, _ *drivers.Config) (bool, server.Backend, error) {
return false, nil, errNoCgo
func postCompactSQL() string {
return `PRAGMA wal_checkpoint(PASSIVE)`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we want a passive checkpoint in the pure-go driver, but full in the cgo driver? The point here is to force a checkpoint and sync the WAL. That may not (and likely will not) happen with a passive checkpoint.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@viscropst can you address this question?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@viscropst are you still interested in this?

@brandond
Copy link
Member

I just merged another smaller PR that changed the sqlite checkpoint stuff a bit to support litestream. Would you mind rebasing and syncing those changes into both versions of the sqlite driver?

@viscropst
Copy link
Author

viscropst commented Jan 23, 2026

I just merged another smaller PR that changed the sqlite checkpoint stuff a bit to support litestream. Would you mind rebasing and syncing those changes into both versions of the sqlite driver?

It's doesn't mind. I'll trying to squash the litestream into both versions.

@viscropst viscropst force-pushed the master branch 4 times, most recently from d5e3b3b to ee9a966 Compare January 23, 2026 11:37
if noCheckpoint {
compactMode = "NOOP"
}
return `PRAGMA wal_checkpoint(` + compactMode + `)`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Why are you changing this from FULL to PASSIVE or NOOP?
  2. Why is the nocgo version missing post-compact checkpoint entirely?

Please restore the checkpoint behavior for both versions.

Introduce the nocgo build to workflow pr

Introduce the nocgo to Dockerfile and build scripts

Fix the `multi-arch-package-nocgo` to use the binary from nocgo build

Introduce the nocgo packge for testing nocgo build

Use the "BIN_SUFFIX" to resolve the variant without cgo in build scripts

Unify the step names for `nocgo` variant

Unify the variable reference usages in `Dockerfile`

Split the sqlite test out of the others

Fix the syntax in pr workflow

Introduce the `nocgo` variant for `Dockerfile.test` building

Change back the job names to `docker` prefixed in pr workflow

Make `docker-nocgo` job load `nocgo` image only

Add back the sqlite test for `docker` job

Introduce `CGO_ENABLED` to `build-api-server-tests`

Introduce the `test` target to `Makefile` for local test

Introduce the `nocgo` variant to release workflow

Fix the missing `-nocgo` tag to `nocgo` variant

Change the built binary name to `kine${BIN_SUFFIX}-${ARCH}`

Signed-off-by: viscropst <viscropst@petalmail.com>
Split the default dsn for nocgo sqlite driver processing

Unify the `dataSourceName` to `getDataSourceName` for both cgo enabled and cgo disabled

Fix the naming and formatting reported by lint check for sqlite driver

Remove the unused `+build cgo` in `sqlite_cgo.go`

Unify the `_busy_timeout` processing to mattn's for easier modify the `busy_timeout`

Use the `PASSIVE` to checkpoint for nocgo variant to skip the busy invokers

Signed-off-by: viscropst <viscropst@petalmail.com>
@viscropst viscropst force-pushed the master branch 3 times, most recently from 786ab5d to 16cd8c5 Compare January 24, 2026 01:40
…`_wal_autocheckpoint` parameter to change checkpoint mode

Signed-off-by: viscropst <viscropst@petalmail.com>
Signed-off-by: viscropst <viscropst@petalmail.com>
Comment on lines +84 to +95
key := "_wal_autocheckpoint"
if params.Has(key) {
if params.Get(key) == "off" || params.Get(key) == "0" || params.Get(key) == "disable" {
return 0
}
if i, err := strconv.Atoi(params.Get(key)); err == nil {
return i
}
}
if params.Has("_kine_disable_wal_autocheckpoint") {
return 0
}
Copy link
Member

@brandond brandond Jan 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. What does the int returned by this function indicate?
  2. Why are you replacing the _kine_disable flag params with _wal_autocheckpoint without any discussion of this change?

Please stop adding changes that are not related to adding the nocgo sqlite driver. This pull request MUST NOT change anything other than adding the new driver, and tests and artifact publishing for same.

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.

It's time to fill out the pure go sqlite driver for better portability

2 participants