PostgreSQL quota manager and storage backend #3644
Conversation
|
/gcbrun |
…cally prepares and caches statements
…tespace in the strings
… without ON DELETE CASCADE exist
…base/sql functions that pgx doesn't have
…TestPostgreSQLURI
There was a problem hiding this comment.
Looking great @robstradling. I've reviewed up to the meaty bits. Looking good so far. A few nitty comments, and then a request for some brief docs to help me understand the flows in the storage layers. Once that's done (and I've had my lunch) I can take a look over the remaining files.
| @@ -49,10 +49,12 @@ import ( | |||
| _ "github.com/google/trillian/storage/cloudspanner" | |||
| _ "github.com/google/trillian/storage/crdb" | |||
| _ "github.com/google/trillian/storage/mysql" | |||
| _ "github.com/google/trillian/storage/postgresql" | |||
There was a problem hiding this comment.
You're following the existing pattern which is the right thing to do. However, I think this pattern is kinda suspect in that it leads to pretty massive binaries. This small change here increases the size of the log server binary by another 10%.
❯ ls -l trillian_log_server
.rwxr-xr-x 61M mhutchinson 30 Oct 11:21 trillian_log_server
❯ go build ./cmd/trillian_log_server
❯ ls -l trillian_log_server
.rwxr-xr-x 67M mhutchinson 30 Oct 11:22 trillian_log_server
I don't expect you to make any code changes in this PR in response to this. But I'm squirting a stream of conciousness into this comment as a TODO to at least raise an issue to discuss this. It could be as simple a solution as docs saying that this binary is the swiss army knife, but if you want a svelte version for a particular environment then copy it and remove the other imports.
There was a problem hiding this comment.
Here's an idea: robstradling#5
|
/gcbrun |
|
/gcbrun |
…ignatureAlgorithm fields
|
/gcbrun |
|
/gcbrun |
There was a problem hiding this comment.
Looking good. The fact that this is tested and passes the tests means that I'm happy with it. Treat my comments here as largely optional if you want the opportunity to reduce the complexity before you bring up any production logs using this. Apart from the error in CODEOWNERS. That will need to be fixed :-)
|
/gcbrun |
|
This is super cool to get PostgreSQL in. Thank you so much for the great contribution, @robstradling! (and thanks @mhutchinson for the review) |
|
Thanks @roger2hk. Adding postgres support for your CTFE 'extra data' storage saving feature is next on my list. :-) |
This PR is based on Trillian's existing MySQL quota manager and storage backend. The first several commits were auto-generated by this script, which forked the existing MySQL code into different directories (whilst preserving the git history) and then did a bunch of search'n'replacing to switch from the database/sql interface to the jackc/pgx interface.
Improving performance is my main reason for using the pgx interface directly. In particular, the pgx interface has allowed me to use PostgreSQL's COPY interface for fast bulk-upserts.
My motivations for putting together this PR are that (1) I and my colleagues at Sectigo have a fair amount of experience with PostgreSQL, but almost no experience with MySQL/MariaDB; and (2) we suffered a CT log failure earlier this year due to MariaDB corruption after disk space exhaustion, and we are confident that PostgreSQL would not have broken under the same circumstances.
Checklist