Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

sql: benchmark the pgbench tcp-b batch #4602

Merged
merged 1 commit into from
Feb 24, 2016
Merged

sql: benchmark the pgbench tcp-b batch #4602

merged 1 commit into from
Feb 24, 2016

Conversation

dt
Copy link
Member

@dt dt commented Feb 23, 2016

standalone pgbench still provides more detailed instrumentation and more configurable options, but a pure-go benchmark is useful too, since its easier to run and integrate with out existing benchmarks.

name                              time/op
PgbenchQuery_Cockroach-8           152µs ± 5%
PgbenchQuery_Postgres-8           63.1µs ± 4%
ParallelPgbenchQuery_Cockroach-8   147µs ± 2%
ParallelPgbenchQuery_Postgres-8   61.9µs ± 2%

name                              alloc/op
PgbenchQuery_Cockroach-8          45.5kB ± 1%
PgbenchQuery_Postgres-8           1.01kB ± 9%
ParallelPgbenchQuery_Cockroach-8  45.7kB ± 1%
ParallelPgbenchQuery_Postgres-8     970B ± 8%

name                              allocs/op
PgbenchQuery_Cockroach-8             884 ± 0%
PgbenchQuery_Postgres-8             27.4 ± 5%
ParallelPgbenchQuery_Cockroach-8     885 ± 0%
ParallelPgbenchQuery_Postgres-8     27.0 ± 0%

Review on Reviewable

tbg
tbg reviewed Feb 23, 2016
View reviewed changes
sql/pgbench_test.go
}

func BenchmarkPgbenchQuery_Cockroach(b *testing.B) {
benchmarkCockroach(b, runBenchmarkBank)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you running the right thing there?

@tbg
Copy link
Member

tbg commented Feb 23, 2016

Unrelated, but do we know where we're losing the time? What happens if you enable the switch in kv/send.go: var enableLocalCalls = os.Getenv("ENABLE_LOCAL_CALLS") == "1"

@dt dt force-pushed the pgbench branch 2 times, most recently from b25ba39 to 5239110 Compare February 23, 2016 20:59
@dt
Copy link
Member Author

dt commented Feb 23, 2016

I had to rebase this on top of #4605 so that one will need to land first.

Review status: 0 of 7 files reviewed at latest revision, 1 unresolved discussion.

sql/pgbench_test.go, line 63 [r1] (raw file):
whoops, fixed.

Comments from the review on Reviewable.io

@petermattis
Copy link
Collaborator

@tschottdorf I think we should make enable local calls the default. Keep having that thought and then forgetting to do so. @dt Definitely test this with ENABLE_LOCAL_CALLS=1.

Review status: 0 of 7 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

Comments from the review on Reviewable.io

@bdarnell
Copy link
Contributor

Review status: 0 of 7 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

sql/pgbench/query.go, line 34 [r3] (raw file):
This comment doesn't match up with the order of variables.

Comments from the review on Reviewable.io

@petermattis
Copy link
Collaborator

Review status: 0 of 7 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

sql/bench_test.go, line 51 [r3] (raw file):
Is there a reason you can't use the existing bench database. See my other comment regarding your change to PGUrl. You could then set pgUrl.Path = "bench".

sql/pgbench/query.go, line 26 [r3] (raw file):
More accurately, the pgwire protocol does not allow for multiple statements in prepared queries.

sql/pgbench/query.go, line 34 [r3] (raw file):
Could you use positional argument syntax to simplify the fmt.Sprintf below:

fmt.Sprintf("%[2]d %[1]d\n", 11, 22)

testutils/sqlutils/pg_url.go, line 68 [r3] (raw file):
I'm not sure we always want to do this. Seems better to let the caller set the path on the returned URL.

Comments from the review on Reviewable.io

@dt dt force-pushed the pgbench branch 2 times, most recently from c339fde to e16fcf6 Compare February 24, 2016 19:27
@dt
Copy link
Member Author

dt commented Feb 24, 2016

Agreed on default change, though I usually don't pass any flags when running numbers for commit messages unless specifically looking at the code the flag changes.

Review status: 0 of 5 files reviewed at latest revision, 5 unresolved discussions.

sql/bench_test.go, line 51 [r3] (raw file):
Done.

sql/pgbench/query.go, line 26 [r3] (raw file):
Done.

sql/pgbench/query.go, line 34 [r3] (raw file):
Wow, that makes it way easier to read. Done.

testutils/sqlutils/pg_url.go, line 68 [r3] (raw file):
Done.

Comments from the review on Reviewable.io

@petermattis
Copy link
Collaborator

Review status: 0 of 5 files reviewed at latest revision, 4 unresolved discussions.

sql/bench_test.go, line 51 [r3] (raw file):
Do you need to create a temporary connection for this to work? Seems like it should work if you create the database as the first operation. Yep, just tested this and it seems to work.

sql/pgbench/setup.go, line 78 [r6] (raw file):
As mentioned in another comment, I don't think you need to create a separate db connection to do this.

Comments from the review on Reviewable.io

@dt
Copy link
Member Author

dt commented Feb 24, 2016

Review status: 0 of 5 files reviewed at latest revision, 4 unresolved discussions.

sql/bench_test.go, line 51 [r3] (raw file):
Huh, I wonder if we're running different versions of postgres? IIRC it errors on connection if you request a DB that doesn't exist?

Comments from the review on Reviewable.io

@petermattis
Copy link
Collaborator

Review status: 0 of 5 files reviewed at latest revision, 4 unresolved discussions.

sql/bench_test.go, line 51 [r3] (raw file):
Ah, postgres may error. I didn't test that. Cockroach doesn't. And this is the cockroach setup so it shouldn't be necessary.

Comments from the review on Reviewable.io

@dt
Copy link
Member Author

dt commented Feb 24, 2016

Review status: 0 of 5 files reviewed at latest revision, 4 unresolved discussions.

sql/bench_test.go, line 51 [r3] (raw file):
Yep, don't need it here, just in the one below that is supposed to work with either.

sql/pgbench/setup.go, line 78 [r6] (raw file):
I do here, since this can be run against postgres.

Comments from the review on Reviewable.io

@petermattis
Copy link
Collaborator

LGTM

Review status: 0 of 5 files reviewed at latest revision, 2 unresolved discussions.

Comments from the review on Reviewable.io

@dt
sql: benchmark the pgbench tcp-b batch
87fd986 
standalone pgbench still provides more detailed instrumentation and
more configurable options, but a pure-go benchmark is useful too,
since its easier to run and integrate with out existing benchmarks.

```
name                              time/op
PgbenchQuery_Cockroach-8          3.16ms ± 1%
PgbenchQuery_Postgres-8            407µs ± 8%
ParallelPgbenchQuery_Cockroach-8  2.45ms ± 9%
ParallelPgbenchQuery_Postgres-8    188µs ±17%

name                              alloc/op
PgbenchQuery_Cockroach-8           230kB ± 0%
ParallelPgbenchQuery_Cockroach-8   246kB ±11%

name                              allocs/op
PgbenchQuery_Cockroach-8           3.48k ± 0%
ParallelPgbenchQuery_Cockroach-8   3.90k ± 7%
```
dt added a commit that referenced this pull request Feb 24, 2016
@dt
Merge pull request #4602 from dt/pgbench
3536d76 
sql: benchmark the pgbench tcp-b batch
@dt dt merged commit 3536d76 into cockroachdb:master Feb 24, 2016
@dt dt deleted the pgbench branch February 24, 2016 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dt @tbg @petermattis @bdarnell