Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upworkload: switch tpcc to pgx and SQLRunner #31465
Conversation
RaduBerinde
requested review from
mjibson,
danhhz and
nvanbenschoten
Oct 16, 2018
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
nvanbenschoten
approved these changes
Oct 16, 2018
nice, I like this structure a lot.
Reviewed 7 of 7 files at r1, 11 of 11 files at r2.
Reviewable status:complete! 0 of 0 LGTMs obtained (and 1 stale)
pkg/workload/pgx_helpers.go, line 128 at r2 (raw file):
) (gosql.Result, error) { _, err := (*pgx.Tx)(tx).ExecEx(ctx, sql, nil /* QueryExOptions */, args...) return nil, err
Leave a comment about why this is ok.
pkg/workload/sql_runner.go, line 115 at r2 (raw file):
panic("already initialized") } sr.name = name
What's the point of this name?
pkg/workload/sql_runner.go, line 206 at r2 (raw file):
// Query executes a query that returns rows. // // See pgx.Tx.Query.
Did you mean to change this?
pkg/workload/tpcc/order_status.go, line 53 at r2 (raw file):
oID int oEntryD time.Time oCarrierID pgtype.Int8
What prompted this change? Just interested.
RaduBerinde
reviewed
Oct 16, 2018
Reviewable status:
complete! 1 of 0 LGTMs obtained
pkg/workload/sql_runner.go, line 115 at r2 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
What's the point of this name?
It's used to generate names for the prepared statements (e.g. kv-1, kv-2`, etc), figured it could help debugging things.
pkg/workload/tpcc/order_status.go, line 53 at r2 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
What prompted this change? Just interested.
We populate this from a result set (by passing it as a pointer to Scan). The implementor of Scan (pgx) needs to know how to work with that type.
nvanbenschoten
reviewed
Oct 16, 2018
Reviewable status:
complete! 1 of 0 LGTMs obtained
pkg/workload/sql_runner.go, line 115 at r2 (raw file):
Previously, RaduBerinde wrote…
It's used to generate names for the prepared statements (e.g.
kv-1, kv-2`, etc), figured it could help debugging things.
Ah ok, but sr.name is never actually used. That's what confused me.
RaduBerinde
reviewed
Oct 16, 2018
TFTR!
Reviewable status:
complete! 0 of 0 LGTMs obtained (and 1 stale)
pkg/workload/pgx_helpers.go, line 128 at r2 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Leave a comment about why this is ok.
Done.
pkg/workload/sql_runner.go, line 115 at r2 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Ah ok, but
sr.nameis never actually used. That's what confused me.
Ah, indeed, removed the field.
pkg/workload/sql_runner.go, line 206 at r2 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Did you mean to change this?
No, fixed.
RaduBerinde
dismissed
nvanbenschoten’s
stale review
Oct 17, 2018
Comments addressed.
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
|
bors r+ |
bot
pushed a commit
that referenced
this pull request
Oct 17, 2018
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
craig
bot
commented
Oct 17, 2018
Build succeeded |
RaduBerinde commentedOct 16, 2018
workload: refactor tpccTx
Refactoring tpccTx to allow per-worker state. This will be necessary
for preparing statements in advance.
Release note: None
workload: use SQLRunner for queries without tuples
Switch TPCC to use pgx; use SQLRunner for queries that have a fixed
number of arguments.
Release note: None
Benchmark results here: https://docs.google.com/spreadsheets/d/13tcnYoRA3ArkZQtEHUMbJOOVdayyhgf43D94fFMiAPY/edit#gid=1942348447
The averages are about the same, but the distribution is very different (p50 through p95 are way better and p99, pMax are much worse). This is probably due to differences in how
pgxandgosqlmanage their connection pools (we have 100 workers using 10 connections).