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
|
/gcbrun |
|
/gcbrun |
|
/gcbrun |
|
/gcbrun |
|
/gcbrun |
|
/gcbrun |
mhutchinson
changed the title
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
| // QuotaManager only implements Global/Write quotas, which is based on the number of Unsequenced | ||
| // rows (to be exact, tokens = MaxUnsequencedRows - actualUnsequencedRows). | ||
| // Other quotas are considered infinite. |
There was a problem hiding this comment.
I suggest moving this to be the second paragraph and expanding it. Other quota managers support features that this doesn't (which is fair enough), but I think should be called out more explicitly and up-front. i.e global only means:
- no tree specific quotas
- no limiting based on intermediary certs
- etc
Phrased another way: this quota manager attempts to protect the MMD SLO of all logs in the instance, but it does not make any attempt to ensure fairness, whether per-tree, per-intermediary, or any other dimension.
There was a problem hiding this comment.
FWIW, this comment was copied directly from quota/mysqlqm/mysql_quota.go.
I'm not familiar with any of the quota managers that offer features that this one doesn't, but I've added text (based heavily on your comment) in 32096c4.
storage/postgresql/admin_storage.go
Outdated
| newTree.TreeId, | ||
| newTree.TreeState.String(), | ||
| newTree.TreeType.String(), | ||
| "RFC6962_SHA256", // Unused, filling in for backward compatibility. |
There was a problem hiding this comment.
For all of these unused/backwards compatible fields, you could simply remove them from the table definition and all the queries. You possibly need to pop them into the protos that get returned, but you don't need them in the schema and DB query level.
It seems a bit unfortunate to be writing a new implementation that carries a legacy burden that isn't needed, but if you're aiming for a minimum diff with MySQL then having them in the DB is fine. Either way, you're the maintainer of this code 🙃
| createTempQueueLeavesTable = "CREATE TEMP TABLE TempQueueLeaves (" + | ||
| " TreeId BIGINT," + | ||
| " LeafIdentityHash BYTEA," + | ||
| " LeafValue BYTEA," + | ||
| " ExtraData BYTEA," + | ||
| " MerkleLeafHash BYTEA," + | ||
| " QueueTimestampNanos BIGINT," + | ||
| " QueueID BYTEA," + | ||
| " IsDuplicate BOOLEAN DEFAULT FALSE" + | ||
| ") ON COMMIT DROP" | ||
| queueLeavesSQL = "SELECT * FROM queue_leaves()" |
There was a problem hiding this comment.
Given enough time I can probably chip away at the layers here to understand what's going on. But it strikes me that what would be super handy is a high level guide in a README.md in this directory that lays out the approach you're taking. If someone were to come and look at this code, how should they start to understand what goes in the temporary tables, the real tables, or the stored procedures? Are the temporary tables bound to a single transaction? Why are stored procedures used rather than direct queries?
I can guess answers to the above with a reasonable degree of accuracy I expect. But I'd feel much more at home if I didn't have to guess 😄 I don't expect this to be a massive tome. Just a couple of paragraphs to explain the approach and perspective you had in mind when this was written will be ace.
|
/gcbrun |
|
/gcbrun |
…ignatureAlgorithm fields
|
/gcbrun |
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