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

exec: finalize vectorize setting name and behavior #38620

Closed
jordanlewis opened this issue Jul 2, 2019 · 1 comment · Fixed by #38777
Closed

exec: finalize vectorize setting name and behavior #38620

jordanlewis opened this issue Jul 2, 2019 · 1 comment · Fixed by #38777
Assignees
Labels
A-sql-vec SQL vectorized engine C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)

Comments

@jordanlewis
Copy link
Member

jordanlewis commented Jul 2, 2019

Currently, the experimental_vectorize flag has 3 values: off, on and always. Off means never run with vectorize, and is the default. On means run everything that vectorized supports with vectorized. Always means run everything with vectorized, even the things that vectorized doesn't support.

This issue tracks changing this setting to what it will look like in 19.2.

We need to change the name of the setting to remove the experimental_ prefix, since anything that's going to be enabled by default shouldn't have experimental in its name.
Then, we need to add a new setting (maybe called on? auto? streaming?) that encodes the behavior of planning vectorized only if all operators are streaming operators.

@jordanlewis jordanlewis added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-sql-vec SQL vectorized engine labels Jul 2, 2019
@yuzefovich
Copy link
Member

yuzefovich commented Jul 10, 2019

There is a lot flakiness with "streaming" on by default on "fakedist" configurations. (Note that I override "vectorize" to off on all fakedist configs.) For example, when I run make testlogic FILES=aggregate three times this is what I get:

            --- FAIL: TestLogic/fakedist-metadata/aggregate/other (0.02s)
                logic.go:2379: 
                    
                    testdata/logic_test/aggregate:173: SELECT json_agg(1) FROM kv
                    expected success, but found
                    (XXUUU) unordered spans (/Table/53/1/{5-8} /Table/53/1/{3-5})
                    kv_batch_fetcher.go:180: in makeKVBatchFetcherWithSendFunc()
        --- PASS: TestLogic/fakedist-metadata/aggregate (1.52s)
            --- PASS: TestLogic/fakedist-metadata/aggregate/other (1.14s)
            --- PASS: TestLogic/fakedist-metadata/aggregate/string_agg (0.19s)
           --- FAIL: TestLogic/fakedist-metadata/aggregate/other (0.05s)
                logic.go:2363: 
                     
                    testdata/logic_test/aggregate:315: SELECT count(*) FROM kv GROUP BY 1+2
                    expected:
                        6
                        
                    but found (query options: "") :
                        9                  

I've seen similar failures on "fakedist" and "fakedist-disk".

My only guess is that there are other "internal" queries that need to be executed in order to run the queries from the logic test files, and those "internal" ones are performed with default vectorize "streaming" setting and are probably accessing some systems tables, and that is broken somehow. Any ideas about that? cc @cockroachdb/sql-execution

There is a bunch of other tests failing at the moment as well, but I have nothing to share about those (maybe it is the same underlying problem).

craig bot pushed a commit that referenced this issue Aug 2, 2019
38777: exec: add "auto" vectorize setting and make it default r=yuzefovich a=yuzefovich

First commit renames Vectorize to VectorizeMode and removes
"experimental_" prefix of vectorize setting.

Second commit adds a fourth option "auto" to vectorize execution mode setting
which plans the queries containing only streaming operators via the
vectorized engine. This option is made a default one. Other options
have been renamed: "on" -> "experimental_on" and "always" ->
"experimental_always" to highlight for users the risk of using them.

Fixes: #38620.
Fixes: #38920.

Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
@craig craig bot closed this as completed in #38777 Aug 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-vec SQL vectorized engine C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants