Add postgres support #135
No reviewers
Labels
No Label
dependencies
duplicate
help wanted
invalid
kind/bug
kind/feature
needs reproduction
question
security
wontfix
No Milestone
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: vikunja/vikunja#135
Loading…
Reference in New Issue
No description provided.
Delete Branch "jtojnar/api:pgsql"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Description
I prefer using PostgreSQL so I tried porting this. Did not test it yet though.
This is basically a first time I have touched Go so please point out any unidiomatic code.
Checklist
go mod vendor
make do-the-swag
)@ -116,0 +145,4 @@
if err != nil {
return
}
engine.SetMaxOpenConns(config.DatabaseMaxOpenConnections.GetInt())
I am not sure if these flags are needed.
I'd say yes, since the max number of open connections can be a limiting factor for performance.
@ -0,0 +1,44 @@
language: go
Why the heck is this vendored too?
That's unfortunatly the way go vendors everything, since the dependencies are only git repositories which are cloned.
The way we're vendoring is not ideal at all, I just haven't gotten around setting up Athens yet.
@ -116,0 +135,4 @@
func initPostgresEngine() (engine *xorm.Engine, err error) {
var connStr strings.Builder
// https://pkg.go.dev/github.com/lib/pq?tab=doc#hdr-Connection_String_Parameters
We will probably need to allow passing the rest of the options. The Go library recommended by xorm decided to stray from libpq’s defaults and forces
sslmode=required
, which fails on my laptop where I do not have TLS set up.Or maybe we can just accept raw connection string?
I'd say we probably should add a setting for
sslmode
.I am not sure if these flags are needed.
I confirmed that this works, at least when the routing works (#137).
Found a bug:
Yup, seems like it. Postgres is a bit more strict about sql statements, unlike mysql or sqlite.
And tests are failing for postgres.
@ -113,6 +120,41 @@ func initMysqlEngine() (engine *xorm.Engine, err error) {
return
}
func postgresConnStrAppend(connStr *strings.Builder, name string, value string) {
This function is super ugly but I am not sure if there is a better way to do this. Maybe iterating over
A much cleaner way would be to use something like
Well, we still want to be able to not pass some of those keys. For example, I connect to the database via socket so username and password does not make sense.
I think we could use something like this:
ad1b6d439f/modules/setting/database.go (L152-L162)
Test failure is caused by the ssl-verify default. That will need to change.
Looking good so far, just some nits.
You can just push as many commits as you want, we'll squash merge at the end.
@ -32,0 +42,4 @@
To restore it, simply pipe it back into the `psql` command:
{{< highlight bash >}}
mysql -U <user> -h <db-host> <database> < vikunja-backup.sql
This should be
psql
.The unit tests are running in ci now (failing), which means we can start fixing the postgresql issues there.
That also includes the bug you had reported. @jtojnar
Most of the tests work now.
Now I'm hitting this: https://dba.stackexchange.com/q/46125
I was able to solve that by changing how the test fixtures are initialized.
@ -9,3 +9,3 @@
- id: 2
text: 'task #2 done'
done: true
done: 1
Would it make sense to use
boolean
in DDL instead? It seems to be supported in all the languages (even though outside of Postgres only as aliases):SQLite
PostgreSQL
MariaDB
I've since changed how the tables are initialized and let xorm handle it based on the type of the field in Go. It seems like for postgres, it creates boolean table definitions. We could change the fixtures back to
true
/false
values now, but the underlying data types in postgres were stilltinyint
s which need either0
or1
.Reverted it since it works with postgres after the other fixes.
@ -116,0 +140,4 @@
var connStr string
host, port := parsePostgreSQLHostPort(config.DatabaseHost.GetString())
if strings.HasPrefix(config.DatabaseHost.GetString(), "/") { // looks like a unix socket
connStr = fmt.Sprintf("postgres://%s:%s@:%s/%s?sslmode=%s&host=%s",
Why not just use the
attr=name
format instead of URI format? Or use this one for both UNIX socket and TCP connection? This only needs to be used for UNIX sockets because URI hostnames cannot contain/
but the other way around is fine.Good point. What do think would be better suited, using the same for both or using the
attr=name
format?I think
attr=%s
is clearer.Then lets change it.
I seems like tests are still failing for unrelated reasons... I'm not sure how to fix that.
The problem seems to be that in postgresql the primary key for id fields is reset each time we're loading test fixtures which makes for duplicate ids...
Looks like there is an official workaround: https://wiki.postgresql.org/wiki/Fixing_Sequences
Tests are passing now (at least the postges errors are gone now) 🎉
I've changed the connection string format and all tests pass, so I guess we can merge this?
@jtojnar Anything to add?
Rebased, merging once all tests pass.