Add postgres support #135

Merged
konrad merged 26 commits from jtojnar/api:pgsql into master 2020-02-16 21:42:05 +00:00
Contributor

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

  • I added or improved tests
  • I pushed new or updated dependencies to the repo using go mod vendor
  • I added or improved docs for my feature
    • Swagger (including make do-the-swag)
    • Error codes
    • New config options
# 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 * [x] I added or improved tests * [x] I pushed new or updated dependencies to the repo using `go mod vendor` * [x] I added or improved docs for my feature * [ ] Swagger (including `make do-the-swag`) * [ ] Error codes * [ ] New config options
jtojnar reviewed 2020-02-15 07:16:34 +00:00
pkg/db/db.go Outdated
@ -116,0 +145,4 @@
if err != nil {
return
}
engine.SetMaxOpenConns(config.DatabaseMaxOpenConnections.GetInt())
Author
Contributor

I am not sure if these flags are needed.

I am not sure if these flags are needed.
Owner

I'd say yes, since the max number of open connections can be a limiting factor for performance.

I'd say yes, since the max number of open connections can be a limiting factor for performance.
jtojnar reviewed 2020-02-15 07:17:53 +00:00
@ -0,0 +1,44 @@
language: go
Author
Contributor

Why the heck is this vendored too?

Why the heck is this vendored too?
Owner

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.

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](https://github.com/gomods/athens) yet.
jtojnar reviewed 2020-02-15 07:50:13 +00:00
pkg/db/db.go Outdated
@ -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
Author
Contributor

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?

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?
Owner

I'd say we probably should add a setting for sslmode.

I'd say we probably should add a setting for `sslmode`.
jtojnar reviewed 2020-02-15 07:51:29 +00:00
pkg/db/db.go Outdated
Author
Contributor

I am not sure if these flags are needed.

I am not sure if these flags are needed.
Author
Contributor

I confirmed that this works, at least when the routing works (#137).

I confirmed that this works, at least when the routing works (#137).
Author
Contributor

Found a bug:

Feb 15 11:07:42 azazel api[156471]: 2020-02-15T11:07:42.193350801+01:00: ERROR        ▶ handler/ReadAllWeb 003 pq: column "label_task.task_id" must appear in the GROUP BY clause or be used in an aggregate function
Feb 15 11:07:42 azazel api[156471]: pq: column "label_task.task_id" must appear in the GROUP BY clause or be used in an aggregate function
Feb 15 11:07:42 azazel api[156471]: 2020-02-15T11:07:42.194172808+01:00: WEB         ▶ ::1  GET 500 /api/v1/labels?page=1 26.582654ms - Mozilla/5.0 (X11; Linux x86_64; rv:72.0) Gecko/20100101 Firefox/72.0
Found a bug: ``` Feb 15 11:07:42 azazel api[156471]: 2020-02-15T11:07:42.193350801+01:00: ERROR ▶ handler/ReadAllWeb 003 pq: column "label_task.task_id" must appear in the GROUP BY clause or be used in an aggregate function Feb 15 11:07:42 azazel api[156471]: pq: column "label_task.task_id" must appear in the GROUP BY clause or be used in an aggregate function Feb 15 11:07:42 azazel api[156471]: 2020-02-15T11:07:42.194172808+01:00: WEB ▶ ::1 GET 500 /api/v1/labels?page=1 26.582654ms - Mozilla/5.0 (X11; Linux x86_64; rv:72.0) Gecko/20100101 Firefox/72.0 ```
Owner

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.

> 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.
jtojnar reviewed 2020-02-15 10:16:02 +00:00
pkg/db/db.go Outdated
@ -113,6 +120,41 @@ func initMysqlEngine() (engine *xorm.Engine, err error) {
return
}
func postgresConnStrAppend(connStr *strings.Builder, name string, value string) {
Author
Contributor

This function is super ugly but I am not sure if there is a better way to do this. Maybe iterating over

params :=map[string]string{
    "user":      config.DatabaseUser.GetString(),
    "password":  config.DatabasePassword.GetString(),
    "host":      config.DatabaseHost.GetString(),
    "dbname":    config.DatabaseDatabase.GetString(),
}
This function is super ugly but I am not sure if there is a better way to do this. Maybe iterating over ``` params :=map[string]string{ "user": config.DatabaseUser.GetString(), "password": config.DatabasePassword.GetString(), "host": config.DatabaseHost.GetString(), "dbname": config.DatabaseDatabase.GetString(), } ```
Owner

A much cleaner way would be to use something like

connStr := fmt.Sprintf("user=%s password=% dbname=%s host=%s", config.DatabaseUser.GetString(), config.DatabasePassword.GetString(), config.DatabaseDatabase.GetString(), config.DatabaseHost.GetString())
A much cleaner way would be to use something like ```golang connStr := fmt.Sprintf("user=%s password=% dbname=%s host=%s", config.DatabaseUser.GetString(), config.DatabasePassword.GetString(), config.DatabaseDatabase.GetString(), config.DatabaseHost.GetString()) ```
Author
Contributor

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.

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.
Owner

I think we could use something like this: ad1b6d439f/modules/setting/database.go (L152-L162)

I think we could use something like this: https://github.com/go-gitea/gitea/blob/ad1b6d439fe0e0875e54227e0bc23a74411f490e/modules/setting/database.go#L152-L162
Author
Contributor

Test failure is caused by the ssl-verify default. That will need to change.

Test failure is caused by the ssl-verify default. That will need to change.
konrad requested changes 2020-02-15 10:28:27 +00:00
konrad left a comment
Owner

Looking good so far, just some nits.

You can just push as many commits as you want, we'll squash merge at the end.

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
Owner

This should be psql.

This should be `psql`.
Owner

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

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
Owner

Most of the tests work now.

Now I'm hitting this: https://dba.stackexchange.com/q/46125

Most of the tests work now. Now I'm hitting this: https://dba.stackexchange.com/q/46125
Owner

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.

> 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.
jtojnar reviewed 2020-02-16 13:27:10 +00:00
@ -9,3 +9,3 @@
- id: 2
text: 'task #2 done'
done: true
done: 1
Author
Contributor

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

sqlite> create table foo(b boolean);
sqlite> insert into foo values(true);
sqlite> select * from foo;
1

PostgreSQL

postgres=# create table foo(b boolean);
CREATE TABLE
postgres=# insert into foo values(true);
INSERT 0 1
postgres=# select * from foo;
 b 
---
 t
(1 row)

MariaDB

MariaDB [test]> create table foo(b boolean);
Query OK, 0 rows affected (0.026 sec)

MariaDB [test]> insert into foo values(true);
Query OK, 1 row affected (0.004 sec)

MariaDB [test]> select * from foo;
+------+
| b    |
+------+
|    1 |
+------+
1 row in set (0.000 sec)
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 ``` sqlite> create table foo(b boolean); sqlite> insert into foo values(true); sqlite> select * from foo; 1 ``` ### PostgreSQL ``` postgres=# create table foo(b boolean); CREATE TABLE postgres=# insert into foo values(true); INSERT 0 1 postgres=# select * from foo; b --- t (1 row) ``` ### MariaDB ``` MariaDB [test]> create table foo(b boolean); Query OK, 0 rows affected (0.026 sec) MariaDB [test]> insert into foo values(true); Query OK, 1 row affected (0.004 sec) MariaDB [test]> select * from foo; +------+ | b | +------+ | 1 | +------+ 1 row in set (0.000 sec) ```
Owner

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 still tinyints which need either 0 or 1.

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 still `tinyint`s which need either `0` or `1`.
Owner

Reverted it since it works with postgres after the other fixes.

Reverted it since it works with postgres after the other fixes.
jtojnar reviewed 2020-02-16 13:48:03 +00:00
pkg/db/db.go Outdated
@ -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",
Author
Contributor

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.

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.
Owner

Good point. What do think would be better suited, using the same for both or using the attr=name format?

Good point. What do think would be better suited, using the same for both or using the `attr=name` format?
Author
Contributor

I think attr=%s is clearer.

I think `attr=%s` is clearer.
Owner

Then lets change it.

Then lets change it.
Owner

I seems like tests are still failing for unrelated reasons... I'm not sure how to fix that.

I seems like tests are still failing for unrelated reasons... I'm not sure how to fix that.
Owner

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

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
Owner

Tests are passing now (at least the postges errors are gone now) 🎉

Tests are passing now (at least the postges errors are gone now) :tada:
Owner

I've changed the connection string format and all tests pass, so I guess we can merge this?

@jtojnar Anything to add?

I've changed the connection string format and all tests pass, so I guess we can merge this? @jtojnar Anything to add?
Owner

Rebased, merging once all tests pass.

Rebased, merging once all tests pass.
konrad closed this pull request 2020-02-16 21:42:05 +00:00
konrad deleted branch pgsql 2020-02-16 21:42:52 +00:00
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: vikunja/vikunja#135
No description provided.