Use wazero-based libpgquery wrapper in currently disabled environments #3027
Conversation
.github/workflows/ci.yml
Outdated
| - run: go build ./... | ||
|
|
||
| darwin-build: | ||
| darwin-test: |
There was a problem hiding this comment.
I realized these may have intentionally been only building and not testing before since there doesn't seem to be a reason for darwin to otherwise fail them. Let me know if I should revert these, but I thought it could be good to verify these work in CI
There was a problem hiding this comment.
FYI I have also filed pganalyze/pg_query_go#103
| // protobuf to unify the types. We must use the wasilibs version because | ||
| // the upstream version requires cgo even when only accessing the proto. | ||
|
|
||
| resBytes, err := proto.Marshal(cgoRes) |
There was a problem hiding this comment.
This is the performance regression, protobuf marshaling is very fast and it's actually relatively common to do this sort of conversion in real code I've seen. But I will try to get go-pgquery to use the same parsetree type as upstream to be able to avoid it, hopefully that goes smoothly
|
@anuraaga Wow, this is really cool. When we talked at GopherCon, I didn't think this was going to work. Very excited to be wrong. Before merging this, I do need to ask about your commitment to maintaining https://github.com/wasilibs/go-pgquery. This will be the only way that PostgreSQL supports works on Windows, so I'd like to ensure that you're comfortable keeping it update with upstream releases. |
|
@anuraaga I cleaned up the ci.yml configuration by using a matrix here: https://github.com/sqlc-dev/sqlc/pull/3035/files That said, the Windows build is failing for both |
|
Thanks @kyleconroy - I subscribed to libpgquery releases and intend to keep updated. One disclaimer is that if the upstream code changed in a way that stops working with the current Wasm build, it could cause delays or blockage. An example would be if it called For windows on the bright side it seems there is interest in official support in libpg_query itself, possibly through Wasm if building with C doesn't seem practical, though I suspect the latter will be sorted out eventually. So at some point the cgo path could probably be pointed back to upstream. pganalyze/pg_query_go#103 (comment) /cc @lfittl
Thanks, I realized I hadn't run the Windows build again after switching to use the upstream proto. I sent pganalyze/pg_query_go#105 to fix it. |
There was a problem hiding this comment.
I have incorporated the fix to allow building with windows and went ahead and brought in the test matrix to this PR to confirm it works. To get some things to pass on Windows I had to make some changes, including in non-test code, so let me know if it's better to remove the test/ci changes from this PR and do them separately.
https://github.com/anuraaga/sqlc/actions/runs/7096959294/job/19316297990
| @@ -46,7 +46,7 @@ func Glob(patterns []string) ([]string, error) { | |||
| files = append(files, filepath.Join(path, f.Name())) | |||
| } | |||
| } else { | |||
| files = append(files, path) | |||
| files = append(files, filepath.Clean(path)) | |||
There was a problem hiding this comment.
The result of filepath.Join is Clean'd, so these two branches didn't have consistent behavior before
| @@ -1,3 +1,7 @@ | |||
| // Currently requires cgo for wasmtime and has line-ending issues on windows. | |||
| //go:build cgo && !windows | |||
There was a problem hiding this comment.
I tried a few things to normalize line endings in the test but ended up giving up since there were some non-trivial issues still
|
Let's keep the CI changes but skip running the tests on Windows. I'm happy to fix that myself, as I'm assuming it's going to be sqlc specific issues. |
After merging #3027, I've been working on getting sqlc to work without CGO. mattn/go-sqlite3 requires CGO, so I've switched to modernc.org/sqlite which does not.
|
This is so cool |
@kyleconroy It was great getting to chat at GopherCon. At the time, I expressed interest in pure-Go sqlc and was running into some blockers due to exception handling mechanisms of postgres. But thanks to the wasix project I was able to get past them - I have been able to create https://github.com/wasilibs/go-pgquery which passes all of both pg_query_go's test cases and libpgquery's tests. It works!
The README is a good place to see caveats - the performance is several times worse, but I wonder if this is ok for a dev tool like sqlc, especially as it goes from unusable to usable and probably not really slow in practice. Let me know if you have any thoughts.
There is an obvious performance regression as I had to settle on a baseline for the parse tree type that doesn't require cgo, I have sent pganalyze/pg_query_go#102 to hopefully address this. It should be unnoticable I think, but a regression is a regression, so I'm definitely happy to wait on this PR until seeing how that goes. I thought it would help either way to get this out to provide context on why anyone would need it and get any initial thoughts.