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.

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

sql: PlaceholderInfo should use slices indexed by placeholder name instead of maps #30086

Closed
nvanbenschoten opened this issue Sep 11, 2018 · 2 comments · Fixed by #33716
Closed
Assignees
Labels
A-sql-pgwire pgwire protocol issues. C-performance Perf of queries or internals. Solution not expected to change functional behavior.

Comments

@nvanbenschoten
Copy link
Member

CockroachDB and PostgreSQL both mandate that all placeholder names are parsable as uint64 values. CockroachDB enforces that this value fits within an int64. Postgres doesn't allow more than 65535 parameters because it uses 16-bit integers for the parameter count in its wire protocol. Furthermore, Postgres does not allow sparse placeholder names:

nathan=# prepare test1 as select $1;
PREPARE
nathan=# prepare test2 as select $2;
ERROR:  could not determine data type of parameter $1

We should adopt this behavior as well.

This strongly implies that we should be storing placeholder names as int16 values instead of string values. More importantly, it implies that PlaceholderInfo should replace its map[string]T maps with slices that can be indexed directly by placeholder name.

This has real performance implications. While running workload init tpcc --warehouses=1000, which performs a large number of batched inserts, I saw that placeholder manipulation was responsible for about 7% of CPU utilization. Most of this time was spent dealing with PlaceholderInfo maps. We should be able to eliminate this overhead almost entirely with a few small changes to how we store placeholders.

screen shot 2018-09-11 at 3 14 35 pm

Assigning @jordanlewis for triage.

@nvanbenschoten nvanbenschoten added C-performance Perf of queries or internals. Solution not expected to change functional behavior. A-sql-pgwire pgwire protocol issues. labels Sep 11, 2018
@nvanbenschoten
Copy link
Member Author

Hah, to add insult to injury, these lines are showing up in heap profiles:

k := strconv.Itoa(i + 1)
qargs[k] = datum

and

k := strconv.Itoa(i + 1)

@jordanlewis
Copy link
Member

This actually is also a crashing bug in Cockroach:

root@127.0.0.1:58327/defaultdb> prepare y as (select $4::int);
PREPARE

Time: 2.973ms

root@127.0.0.1:58327/defaultdb> execute y(1);
E181228 21:28:21.112462 595 sql/conn_executor.go:661  [n1,client=127.0.0.1:58329,user=root] a SQL panic has occurred while executing "EXECUTE y (1)": the desired type for tree.TypeCheck cannot be nil, use types.Any instead
E181228 21:28:21.112677 595 util/log/crash_reporting.go:202  [n1,client=127.0.0.1:58329,user=root] a panic has occurred!
E181228 21:28:21.114886 595 util/log/crash_reporting.go:476  [n1,client=127.0.0.1:58329,user=root] Reported as error 4a36eccba7ff49008f847e48f7896374
panic: the desired type for tree.TypeCheck cannot be nil, use types.Any instead [recovered]
        panic: panic while executing 1 statements: EXECUTE _ (_); caused by the desired type for tree.TypeCheck cannot be nil, use types.Any instead

goroutine 595 [running]:
github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).closeWrapper(0xc001025600, 0x714f380, 0xc001679e80, 0x6539060, 0x710ae00)
        /Users/jordan/repo/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:675 +0x36d
github.com/cockroachdb/cockroach/pkg/sql.(*Server).ServeConn.func1(0xc001025600, 0x714f380, 0xc001679e80)
        /Users/jordan/repo/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:407 +0x61
panic(0x6539060, 0x710ae00)
        /usr/local/Cellar/go/1.11/libexec/src/runtime/panic.go:513 +0x1b9
github.com/cockroachdb/cockroach/pkg/sql/sem/tree.TypeCheck(0x7153480, 0xc000807340, 0xc0004d7320, 0x0, 0x0, 0x0, 0x0, 0xc0004d7320, 0x90)
        /Users/jordan/repo/src/github.com/cockroachdb/cockroach/pkg/sql/sem/tree/type_check.go:260 +0x101
github.com/cockroachdb/cockroach/pkg/sql/sqlbase.SanitizeVarFreeExpr(0x7153480, 0xc000807340, 0x0, 0x0, 0x69f72e3, 0x11, 0xc0004d7320, 0x0, 0x8954a01, 0x0, ...)
        /Users/jordan/repo/src/github.com/cockroachdb/cockroach/pkg/sql/sqlbase/table.go:57 +0x174
github.com/cockroachdb/cockroach/pkg/sql.fillInPlaceholders(0xc0004d7560, 0x8954a39, 0x1, 0xc0017360c0, 0x1, 0x1, 0xc0002e3a80, 0x1, 0x1, 0x0, ...)
        /Users/jordan/repo/src/github.com/cockroachdb/cockroach/pkg/sql/execute.go:44 +0x323
github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execStmtInOpenState(0xc001025600, 0x714f440, 0xc0011289f0, 0xc00175e72b, 0xc, 0x7153080, 0xc0011287e0, 0x0, 0x0, 0x0, ...)
        /Users/jordan/repo/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:317 +0x1961
github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execStmt(0xc001025600, 0x714f440, 0xc0011289f0, 0xc00175e72b, 0xc, 0x7153080, 0xc0011287e0, 0x0, 0x0, 0x0, ...)
        /Users/jordan/repo/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:98 +0x33f
github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).run(0xc001025600, 0x714f380, 0xc001679e80, 0xc000804c38, 0x5400, 0x15000, 0xc000804cd0, 0xc0002e3a90, 0x0, 0x0)
        /Users/jordan/repo/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1141 +0x2154
github.com/cockroachdb/cockroach/pkg/sql.(*Server).ServeConn(0xc0008a7200, 0x714f380, 0xc001679e80, 0xc001025600, 0x5400, 0x15000, 0xc000804cd0, 0xc0002e3a90, 0x0, 0x0)
        /Users/jordan/repo/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:409 +0xce
github.com/cockroachdb/cockroach/pkg/sql/pgwire.(*conn).serveImpl.func4(0xc0008a7200, 0x714f380, 0xc001679e80, 0xc001025600, 0x5400, 0x15000, 0xc000804cd0, 0xc0002e3a90, 0xc0002e3aa0, 0xc000412f84)
        /Users/jordan/repo/src/github.com/cockroachdb/cockroach/pkg/sql/pgwire/conn.go:319 +0x81
created by github.com/cockroachdb/cockroach/pkg/sql/pgwire.(*conn).serveImpl
        /Users/jordan/repo/src/github.com/cockroachdb/cockroach/pkg/sql/pgwire/conn.go:318 +0x1033

RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Jan 4, 2019
This change modifies `tree.Placeholder` to store an integer ID
(between 1 and 65535) instead of a string. This saves silly
allocations in pgwire, and in a future change we can switch the
PlaceholderInfo maps to slices.

Informs cockroachdb#30086.

Release note: None
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Jan 4, 2019
This change modifies `tree.Placeholder` to store an integer ID
(between 1 and 65535) instead of a string. This saves silly
allocations in pgwire, and in a future change we can switch the
PlaceholderInfo maps to slices.

Informs cockroachdb#30086.

Release note: None
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Jan 5, 2019
This change modifies `tree.Placeholder` to store an integer ID
(between 0 and 65535) instead of a string. This saves silly
allocations in pgwire, and in a future change we can switch the
PlaceholderInfo et al maps to slices.

Informs cockroachdb#30086.

Release note: None
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Jan 5, 2019
This change modifies `tree.Placeholder` to store an integer ID
(between 0 and 65535) instead of a string. This saves silly
allocations in pgwire, and in a future change we can switch the
PlaceholderInfo et al maps to slices.

Informs cockroachdb#30086.

Release note: None
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Jan 6, 2019
This change modifies `tree.Placeholder` to store an integer ID
(between 0 and 65535) instead of a string. This saves silly
allocations in pgwire, and in a future change we can switch the
PlaceholderInfo et al maps to slices.

Informs cockroachdb#30086.

Release note: None
craig bot pushed a commit that referenced this issue Jan 6, 2019
33515: sql: use integer placeholder IDs r=RaduBerinde a=RaduBerinde

This change modifies `tree.Placeholder` to store an integer ID
(between 1 and 65535) instead of a string. This saves silly
allocations in pgwire, and in a future change we can switch the
PlaceholderInfo maps to slices.

Informs #30086.

Release note: None

Co-authored-by: Radu Berinde <radu@cockroachlabs.com>
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Jan 15, 2019
We switch `PlaceholderTypes` to a slice instead of a map.

We also fix the handling of cases where some placeholders are unused
(e.g. `SELECT $2:::int`) which now error out (before they would crash
during execution). Note that PG also errors out in this case.

Fixes cockroachdb#30086.

Release note (bug fix): Preparing queries with missing placeholders
(e.g. `SELECT $2::int`) now results in an error.
craig bot pushed a commit that referenced this issue Jan 16, 2019
33716: sql: switch QueryArguments, PlaceholderTypes to slices r=RaduBerinde a=RaduBerinde

#### sql: change QueryArguments map to a slice

Release note: None

#### sql: switch PlaceholderTypes to a slice

We switch `PlaceholderTypes` to a slice instead of a map.

We also fix the handling of cases where some placeholders are unused
(e.g. `SELECT $2:::int`) which now error out (before they would crash
during execution). Note that PG also errors out in this case.

Fixes #30086.

Release note (bug fix): Preparing queries with missing placeholders
(e.g. `SELECT $2::int`) now results in an error.


Co-authored-by: Radu Berinde <radu@cockroachlabs.com>
@craig craig bot closed this as completed in #33716 Jan 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-pgwire pgwire protocol issues. C-performance Perf of queries or internals. Solution not expected to change functional behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants