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: add tests for CTAS, CMVAS with every SHOW and vtable #105895

Closed
ecwall opened this issue Jun 30, 2023 · 0 comments · Fixed by #106407
Closed

sql: add tests for CTAS, CMVAS with every SHOW and vtable #105895

ecwall opened this issue Jun 30, 2023 · 0 comments · Fixed by #106407
Assignees
@ecwall
Labels
A-sql-vtables Virtual tables - pg_catalog, information_schema etc C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) db-cy-23 T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@ecwall
Copy link
Contributor

ecwall commented Jun 30, 2023 •
edited
Loading

Some support issues have been created related to CREATE TABLE AS, CREATE MATERIALIZED VIEW AS when sourcing from SHOW or a vtable:
#76710
https://github.com/cockroachlabs/support/issues/2428
https://github.com/cockroachlabs/support/issues/2394

We can preempt these tickets by exhaustively testing all combinations of CREATE AS with SHOW / vtable.

If any problems are found during these tests, we can exclude the failing combinations and create issues to investigate them.

Jira issue: CRDB-29262
@ecwall ecwall added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-sql-vtables Virtual tables - pg_catalog, information_schema etc T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) labels Jun 30, 2023
@ecwall ecwall self-assigned this Jun 30, 2023
@ecwall ecwall mentioned this issue Jul 5, 2023
Merged
craig bot pushed a commit that referenced this issue Jul 5, 2023
@nvanbenschoten @ecwall
Merge #106155 #106157
e337ad7 
106155: sql/sqlstats: avoid heap allocations when using stats iterators r=maryliag a=nvanbenschoten

This PR contains two changes that each avoid per-transaction heap allocations in the `sqlstats` package.

### sql/sqlstats: return iterators by value from constructors

This commit switches to returning sqlstats iterations by value from their constructors, instead of by pointer. This avoids causing the iterators to escape and needing to allocate on the heap. Instead, they can be kept on the stack, avoiding heap allocations in methods like `IterateStatementStats`.

```sh
➜ benchdiff --run='BenchmarkKV/./SQL/rows=1$$' --count=25 ./pkg/sql/tests

name                     old time/op    new time/op    delta
KV/Insert/SQL/rows=1-10     173µs ±13%     172µs ±15%    ~     (p=0.939 n=25+25)
KV/Update/SQL/rows=1-10     285µs ±16%     280µs ±11%    ~     (p=0.356 n=25+23)
KV/Delete/SQL/rows=1-10     212µs ±25%     221µs ±19%    ~     (p=0.345 n=25+25)
KV/Scan/SQL/rows=1-10       127µs ±11%     127µs ±11%    ~     (p=0.878 n=25+24)

name                     old alloc/op   new alloc/op   delta
KV/Scan/SQL/rows=1-10      32.2kB ± 1%    32.0kB ± 1%  -0.36%  (p=0.006 n=24+24)
KV/Insert/SQL/rows=1-10    59.0kB ± 1%    58.9kB ± 1%    ~     (p=0.140 n=25+25)
KV/Update/SQL/rows=1-10    70.6kB ± 1%    70.4kB ± 1%    ~     (p=0.050 n=24+24)
KV/Delete/SQL/rows=1-10    84.9kB ± 1%    84.9kB ± 1%    ~     (p=0.859 n=25+25)

name                     old allocs/op  new allocs/op  delta
KV/Scan/SQL/rows=1-10         368 ± 1%       366 ± 1%  -0.62%  (p=0.001 n=24+24)
KV/Update/SQL/rows=1-10       726 ± 1%       723 ± 2%  -0.45%  (p=0.013 n=23+24)
KV/Insert/SQL/rows=1-10       492 ± 2%       490 ± 1%  -0.41%  (p=0.044 n=25+25)
KV/Delete/SQL/rows=1-10       613 ± 3%       613 ± 3%    ~     (p=0.179 n=25+25)
```

### sql/sqlstats: pass `sqlstats.IteratorOptions` by value, not pointer

This commit switches from passing the `sqlstats.IteratorOptions` around by pointer to passing it by value. The struct is 2 bytes large (8 when padded), so there's little benefit to passing it around by pointer. Meanwhile, passing the object by pointer through interface methods prevents escape analysis from keeping it on the stack, forcing a heap allocation.

```
➜ benchdiff --run='BenchmarkKV/./SQL/rows=1$$' --count=25 ./pkg/sql/tests

name                     old time/op    new time/op    delta
KV/Insert/SQL/rows=1-10     169µs ±13%     168µs ±17%    ~     (p=0.603 n=25+25)
KV/Update/SQL/rows=1-10     282µs ± 9%     282µs ± 8%    ~     (p=0.788 n=25+25)
KV/Delete/SQL/rows=1-10     227µs ±26%     206µs ±27%    ~     (p=0.074 n=25+25)
KV/Scan/SQL/rows=1-10       126µs ±12%     127µs ±16%    ~     (p=0.908 n=25+25)

name                     old alloc/op   new alloc/op   delta
KV/Delete/SQL/rows=1-10    84.9kB ± 1%    84.5kB ± 1%  -0.50%  (p=0.008 n=25+21)
KV/Insert/SQL/rows=1-10    58.9kB ± 1%    58.7kB ± 1%  -0.23%  (p=0.020 n=25+23)
KV/Update/SQL/rows=1-10    70.5kB ± 1%    70.5kB ± 1%    ~     (p=0.894 n=24+24)
KV/Scan/SQL/rows=1-10      32.0kB ± 1%    32.0kB ± 1%    ~     (p=0.631 n=25+24)

name                     old allocs/op  new allocs/op  delta
KV/Delete/SQL/rows=1-10       613 ± 3%       604 ± 1%  -1.52%  (p=0.000 n=25+21)
KV/Insert/SQL/rows=1-10       489 ± 1%       486 ± 1%  -0.69%  (p=0.001 n=25+23)
KV/Scan/SQL/rows=1-10         365 ± 1%       363 ± 1%  -0.42%  (p=0.024 n=25+25)
KV/Update/SQL/rows=1-10       724 ± 2%       722 ± 2%    ~     (p=0.302 n=25+24)
```

Epic: None
Release note: None

106157:  sql: add tests for CTAS, CMVAS with every vtable r=chengxiong-ruan a=ecwall

Informs #105895

Adds tests for CREATE TABLE AS, CREATE MATERIALIZED VIEW AS sourcing from
all vtables.

Release note: None

Co-authored-by: Nathan VanBenschoten <[email protected]>
Co-authored-by: Evan Wall <[email protected]>
@blathers-crl blathers-crl bot mentioned this issue Jul 5, 2023
Merged
blathers-crl bot pushed a commit that referenced this issue Jul 5, 2023
@ecwall
sql: add tests for CTAS, CMVAS with every vtable
24b2229 
Informs #105895

Adds tests for CREATE TABLE AS, CREATE MATERIALIZED VIEW AS sourcing from
all vtables.

Release note: None
@ecwall ecwall mentioned this issue Jul 5, 2023
Merged
craig bot pushed a commit that referenced this issue Jul 5, 2023
@knz @ecwall
Merge #106122 #106203
e8aedca 
106122: server: use HTTP query parameter `cluster` for manual selection r=yuzefovich a=knz

Informs #106068.
Epic: CRDB-29380

Prior to this patch, there was a debug-only way to manually force a HTTP request to be routed to a particular virtual cluster through the server controller. This was achieved via the query parameter `tenant_name`.

This patch renames the paramater to `cluster`, for a better UX coherence with the option of the same name in `cockroach sql`.

Release note: None

106203: sql: fix CREATE AS sourcing vtable panics r=chengxiong-ruan a=ecwall

Fixes #106166
Fixes #106167
Fixes #106168
Informs #105895

Fixes panics affects CREATE AS sourcing from vtables. These statements now work properly:
```
CREATE TABLE t AS SELECT * FROM pg_catalog.pg_prepared_statements;
CREATE MATERIALIZED VIEW v AS SELECT * FROM pg_catalog.pg_prepared_statements;
CREATE TABLE t AS SELECT * FROM pg_catalog.pg_cursors;
CREATE MATERIALIZED VIEW v AS SELECT * FROM pg_catalog.pg_cursors;
CREATE TABLE t AS SELECT * FROM crdb_internal.create_statements;
CREATE MATERIALIZED VIEW v AS SELECT * FROM crdb_internal.create_statements;
```

Co-authored-by: Raphael 'kena' Poss <[email protected]>
Co-authored-by: Evan Wall <[email protected]>
This was referenced Jul 6, 2023
Merged
Merged
craig bot pushed a commit that referenced this issue Jul 7, 2023
@ecwall @yuzefovich @chengxiong-ruan
Merge #106072 #106238 #106320 #106407 #106420
38cd5c4 
106072: server: close net.Conn unconditionally after accepting r=rafiss,knz a=ecwall

Informs #105448

Previously after the server accepted a connection, it was closed in multiple paths and in multiple layers after it was done being used. Now it is always closed in the same layer that accepted it after the serveConn callback returns.

Release note: None

106238: server: fix crdb_internal.cluster_inflight_traces in shared process mode r=yuzefovich a=yuzefovich

This commit fixes a panic that would previously occur when querying `crdb_internal.cluster_inflight_traces` virtual table when running in shared-process multi-tenant mode. In particular, the problem was that we tried to access node liveness which isn't available, and now we will fall back to the multi-tenant way of doing things (using the instances reader). Additionally, this commit extends the existing test to also run in shared-process multi-tenant config which serves as a regression test for this bug. There is no release note since it's not a user-visible bug.

Fixes: #106182.
Epic: CRDB-26691

Release note: None

106320: sql: unskip TestSchemaChangeGCJob r=chengxiong-ruan a=chengxiong-ruan

Informs: #85876
fixes #60664

A few assumptions has been changed since the test was skipped. For example, the first user defined table descriptor ID and expected error messages. There was actually also bugs in the error assertion in the test for some reason, that is fixed by this PR too.

Release note: None

106407: sql: add tests for CTAS, CMVAS with every SHOW statement r=chengxiong-ruan a=ecwall

Fixes #105895

Adds tests for CREATE TABLE AS, CREATE MATERIALIZED VIEW AS sourcing from
all SHOW statements.

Release note: None


106420: server: unskip TestDatabasesTablesV2 r=THardy98 a=THardy98

Resolves: #87074

This test no longer seems flaky after testing (via `./dev test pkg/server --filter=TestDatabasesTablesV2 --stress --stress-args="-p=4" --race --timeout=30m` on gceworker).

Release note: None

Co-authored-by: Evan Wall <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Chengxiong Ruan <[email protected]>
Co-authored-by: Thomas Hardy <[email protected]>
@craig craig bot closed this as completed in 576bb93 Jul 7, 2023
@blathers-crl blathers-crl bot mentioned this issue Jul 7, 2023
Closed
blathers-crl bot pushed a commit that referenced this issue Jul 7, 2023
@ecwall
sql: add tests for CTAS, CMVAS with every SHOW statement
54cac8d 
Fixes #105895

Adds tests for CREATE TABLE AS, CREATE MATERIALIZED VIEW AS sourcing from
all SHOW statements.

Release note: None
@blathers-crl blathers-crl bot mentioned this issue Jul 7, 2023
Merged
blathers-crl bot pushed a commit that referenced this issue Jul 7, 2023
@ecwall
sql: add tests for CTAS, CMVAS with every SHOW statement
8b20b57 
Fixes #105895

Adds tests for CREATE TABLE AS, CREATE MATERIALIZED VIEW AS sourcing from
all SHOW statements.

Release note: None
@ecwall ecwall mentioned this issue Jul 19, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ecwall ecwall

Labels
A-sql-vtables Virtual tables - pg_catalog, information_schema etc C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) db-cy-23 T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

sql: add tests for CTAS, CMVAS with every SHOW statement
1 participant
@ecwall