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: remove vectorize=201auto option #55907

Merged
merged 1 commit into from
Oct 27, 2020

Conversation

yuzefovich
Copy link
Member

@yuzefovich yuzefovich commented Oct 23, 2020

201auto option of vectorize setting has been removed since it no
longer makes sense to keep (it was introduced as an escape hatch for
20.2 release). Note that we don't need to bump the distsql version
because of this change since it is backwards compatible - if the gateway
is running the old version that has vectorize=201auto set, then we
will check whether flows for all nodes don't have non-streaming
operators and possibly use off option on the flow setup request, then
the newer version remote node will check the vectorize setting on the
request whether it is not off and setup the vectorized flow if it is
not.

Release note (sql change): 201auto value for vectorize session
variable and the corresponding cluster setting has been removed.

@yuzefovich yuzefovich requested review from asubiotto and a team October 23, 2020 18:38
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@yuzefovich yuzefovich force-pushed the remove-201auto branch 2 times, most recently from e0fbd46 to 7d5edc5 Compare October 24, 2020 20:44
Copy link
Contributor

@asubiotto asubiotto left a comment

Choose a reason for hiding this comment

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

What about when the gateway is using the new version, and the other node is running the old version? Will that node run a streaming-only vectorized flow?

Reviewed 16 of 16 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

@yuzefovich
Copy link
Member Author

Note that the decision whether to vectorize the whole flow or not is made on the gateway, and then each node participating in the plan checks simply whether Vectorize setting from evalCtx proto is off or not to decide which flow to set up on itself.

In your example, say the newer node will set Vectorize=on, and the remote nodes running older version will also see on option, so they won't pay attention to the fact whether the operators are streaming or not.

@yuzefovich yuzefovich force-pushed the remove-201auto branch 2 times, most recently from 15bf9a5 to 60156fb Compare October 27, 2020 01:57
Copy link
Contributor

@asubiotto asubiotto left a comment

Choose a reason for hiding this comment

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

:lgtm:

In your example, say the newer node will set Vectorize=on, and the remote nodes running older version will also see on option, so they won't pay attention to the fact whether the operators are streaming or not.

Ahh gotcha, I was missing that you reserved the Vectorize201Auto field in the evalctx proto

Reviewed 2 of 2 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)


pkg/sql/colflow/BUILD.bazel, line 26 at r2 (raw file):

        "//pkg/sql/pgwire/pgerror",
        "//pkg/sql/rowexec",
        "//pkg/sql/sessiondatapb",

Is this removal correct?

Copy link
Contributor

@asubiotto asubiotto left a comment

Choose a reason for hiding this comment

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

Hit the block by mistake

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)

`201auto` option of `vectorize` setting has been removed since it no
longer makes sense to keep (it was introduced as an escape hatch for
20.2 release). Note that we don't need to bump the distsql version
because of this change since it is backwards compatible - if the gateway
is running the old version that has `vectorize=201auto` set, then we
will check whether flows for all nodes don't have non-streaming
operators and possibly use `off` option on the flow setup request, then
the newer version remote node will check the vectorize setting on the
request whether it is not `off` and setup the vectorized flow if it is
not.

Release note (sql change): `201auto` value for `vectorize` session
variable and the corresponding cluster setting has been removed.
Copy link
Member Author

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

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

TFTR!

bors r+

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @asubiotto)


pkg/sql/colflow/BUILD.bazel, line 26 at r2 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

Is this removal correct?

Yeah - we no longer use sessiondatapb package in vectorized_flow file, so this package no longer needs to be imported when building colflow package.

@craig
Copy link
Contributor

craig bot commented Oct 27, 2020

Build succeeded:

@craig craig bot merged commit b319a0b into cockroachdb:master Oct 27, 2020
@yuzefovich yuzefovich deleted the remove-201auto branch October 27, 2020 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants