Improves Postgres write performance #852
Conversation
github-actions
bot
added
area/datastore
jakedt
force-pushed
the
postgres-perf
branch
from
2fc97bf
to
5aafa30
Compare
jakedt
force-pushed
the
postgres-perf
branch
3 times, most recently
from
01194d1
to
55d16d2
Compare
internal/datastore/postgres/migrations/zz_migration.0009_add_xid8_columns.go
Outdated
Show resolved
Hide resolved
internal/datastore/postgres/migrations/zz_migration.0009_add_xid8_columns.go
Outdated
Show resolved
Hide resolved
| @@ -32,7 +32,7 @@ func RunPostgresForTesting(t testing.TB, bridgeNetworkName string) RunningEngine | |||
| resource, err := pool.RunWithOptions(&dockertest.RunOptions{ | |||
| Name: name, | |||
| Repository: "postgres", | |||
| Tag: "10.20", | |||
| Tag: "13.8", | |||
There was a problem hiding this comment.
I think it might be worth moving this version into a const in the postgres datastore itself
There was a problem hiding this comment.
I think the min required version (13.0) and the test version (latest 13.x) aren't necessarily the same? I can see postgres advertising its min required version.
| if err := dbpool. | ||
| QueryRow(initializationContext, "SHOW track_commit_timestamp;"). | ||
| Scan(&trackTSOn); err != nil { | ||
| return nil, fmt.Errorf(errUnableToInstantiate, err) |
There was a problem hiding this comment.
I think we need a more descriptive error here if this feature is not enabled, ideally one indicating the exact documentation on how to enable it
There was a problem hiding this comment.
Do we want to skip this check if watch is disabled?
There was a problem hiding this comment.
The datastore has no idea if the watch server is enabled.
There was a problem hiding this comment.
Maybe we should thread that in then?
| @@ -74,7 +78,7 @@ func init() { | |||
| var ( | |||
| psql = sq.StatementBuilder.PlaceholderFormat(sq.Dollar) | |||
|
|
|||
| getRevision = psql.Select("MAX(id)").From(tableTransaction) | |||
| getRevision = psql.Select("MAX(xid::text::bigint)").From(tableTransaction) | |||
There was a problem hiding this comment.
Should this use a const name for the col name? I know we didn't use it before, but maybe we should now?
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
jakedt
force-pushed
the
postgres-perf
branch
from
55d16d2
to
8765b28
Compare
There was a problem hiding this comment.
It took me a bit to process all the changes commit by commit, but I think this looks pretty solid to me. I think the fundamental question to clarify is what kind of impact these migrations can have in a production PSQL primary, and describing what the the various manual steps OSS users have to take to migrate their PSQL datastores to the this version are.
Great job @jakedt this is good stuff 👏🏻 👏🏻 👏🏻
internal/datastore/postgres/migrations/zz_migration.0011_add_xid_indices.go
Outdated
Show resolved
Hide resolved
internal/datastore/postgres/migrations/zz_migration.0012_add_xid_constraints.go
Outdated
Show resolved
Hide resolved
| @@ -44,14 +44,14 @@ const ( | |||
| // %[4] Inverse of GC window (in seconds) | |||
| queryValidTransaction = ` | |||
| SELECT $1 >= ( | |||
| SELECT MIN(%[1]s) FROM %[2]s WHERE %[3]s >= NOW() - INTERVAL '%[4]f seconds' | |||
| SELECT MIN(%[1]s::text::bigint) FROM %[2]s WHERE %[3]s >= NOW() - INTERVAL '%[4]f seconds' | |||
There was a problem hiding this comment.
nit: perhaps we could have turned this type cast into a PSQL function for readability
Previously Postgres was limited to 1 concurrent write request globally. With this change we leverage some of the transaction isolation primitives introduced in Postgres 13 to extend the effective repeatable read lifetime beyond the transaction's lifetime. With this we can leverage internal Postgres transaction IDs and snapshots to greatly improve the performance of our MVCC implementation.
Contains a 4-phase migration!