PostgreSQL storage #50
Conversation
|
PostgreSQL integration test results |
There was a problem hiding this comment.
Thanks for this! Overall looks good, but there are a few changes requested. In general I'm fine with the copy/paste, so no worries there.
A few questions/requests:
- Could we rename the module to
pgsqlfrompostgresql(and same with the backend name for the CLI)? - Could you add docs to the
docs/operations-guide.mdfile? - Could you add in the deleter functionality from Optionally delete commands in mysql storage backend #48? (This will get merged soon)
- Thanks for running the integration tests and adding screenshots, that's helpful!
- Have you run through actual tests with "real" devices and such? There's also mdmb if you want to play with that.
Again thanks for this!
storage/postgresql/certauth.go
Outdated
| } | ||
|
|
||
| // AssociateCertHash | ||
| // TODO primary key consists of two to cols: id & sha256 |
There was a problem hiding this comment.
I think the reason for this in the mysql backend was to have a way to ignore/bypass errors if the insert already exists. I can't think of a good reason why we're updating sha256 to the same value it already is. I.e. so either an insert or update will succeed silently. Perhaps the better thing is to just insert and check for the the duplicate error (and ignore it)?
There was a problem hiding this comment.
The solution for PostgreSQL is:
ON CONFLICT ON CONSTRAINT cert_auth_associations_pkey DO NOTHING
There was a problem hiding this comment.
INSERT IGNORE - possible solution for MySQL. What do you think?
storage/postgresql/postgresql.go
Outdated
| serial_number = EXCLUDED.serial_number, | ||
| authenticate = EXCLUDED.authenticate, | ||
| authenticate_at = CURRENT_TIMESTAMP;`, | ||
| r.ID, nullEmptyString(string(pemCert)), nullEmptyString(msg.SerialNumber), msg.Raw, |
There was a problem hiding this comment.
| r.ID, nullEmptyString(string(pemCert)), nullEmptyString(msg.SerialNumber), msg.Raw, | |
| r.ID, pemCert, nullEmptyString(msg.SerialNumber), msg.Raw, |
Does pemCert need to be cast to string? For the MySQL driver the nil/zero-value []byte gets converted into a NULL column.
There was a problem hiding this comment.
Unfortunately, pg driver converts nil bytes slice into an empty string, but not NULL. Tested additionally this functionality. So casting here is the simplest solution.
storage/postgresql/schema.sql
Outdated
| enabled BOOLEAN NOT NULL DEFAULT TRUE, | ||
| token_update_tally INTEGER NOT NULL DEFAULT 1, | ||
|
|
||
| last_seen_at TIMESTAMP NOT NULL, -- DEFAULT CURRENT_TIMESTAMP, tests pass, but real test push error |
There was a problem hiding this comment.
When I've just started working, I tried to test with MySQL DB. And after connecting with iPhone, I've received such log message:
while pushing got error: handler=checkin-command msg=check-in request err=tokenupdate service: Error 1364: Field 'last_seen_at' doesn't have a default value
After forking I've changed in MySQL schema:
last_seen_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP,
and the real device worked fine. I've forgotten about this, copied MySQL package, and started working on PostgreSQL.
After finishing, I started integration tests, but in MySQL they failed because of DEFAULT CURRENT_TIMESTAMP.
I rolled back to the original in MySQL - and integration tests passed ok.
I think such a situation needs some investigation :))
Co-authored-by: Jesse Peterson <jessepeterson@users.noreply.github.com>
Co-authored-by: Jesse Peterson <jessepeterson@users.noreply.github.com>
Co-authored-by: Jesse Peterson <jessepeterson@users.noreply.github.com>
|
Thanks, Jesse for your review! I'll do the requested changes. |
|
I've done the requested changes, except "deleter" functionality. |
|
|
|
The latest commit includes the "deleter" functionality of pgsql, and MySQL from #48 |
PostgreSQL storage added.
Based on mysql storage package. Much of the code is duplicated, but as Go-authors said, sometimes a little more copy-paste is better than a little more dependencies.
github.com/lib/pq - is used as db driver.