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

release-20.2: sql: disable interleaved joins by default #54162

Merged
merged 1 commit into from
Sep 18, 2020

Conversation

yuzefovich
Copy link
Member

This commit switches the default value of a cluster setting to disable
planning the interleaved joins because they are under-tested and often
are actually slower than the merge joins. In 21.1 release we will be
removing the corresponding code entirely.

Release note (sql change): Interleaved joins are now disabled by default
and will be entirely removed in 21.1 release because they are often
slower than the merge join.

@yuzefovich yuzefovich requested review from asubiotto and a team September 10, 2020 03:32
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@yuzefovich yuzefovich changed the title sql: disable interleaved joins by default release-20.2: sql: disable interleaved joins by default Sep 10, 2020
@asubiotto
Copy link
Contributor

LGTM pending review in release meeting. There are also a bunch of logictests that need to be cleaned up.

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.

Fixed the tests.

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


pkg/sql/logictest/testdata/logic_test/select_for_update_nowait_interleaved, line 10 at r1 (raw file):

# logic of mapping a WriteIntentError back to the corresponding table.

# Note that for some reason setting a session variable is not sufficient to

@nvanbenschoten do you know why SET enable_interleaved_joins = true; would work differently from the cluster setting change?

Copy link
Member

@nvanbenschoten nvanbenschoten left a comment

Choose a reason for hiding this comment

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

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


pkg/sql/logictest/testdata/logic_test/select_for_update_nowait_interleaved, line 10 at r1 (raw file):

Previously, yuzefovich wrote…

@nvanbenschoten do you know why SET enable_interleaved_joins = true; would work differently from the cluster setting change?

It could be because we're switching users down below, which I believe means that we're creating a new session. Setting a session variable only impacts the current session, while setting the cluster setting impacts the initial value of the session variables for all sessions.

This commit switches the default value of a cluster setting to disable
planning the interleaved joins because they are under-tested and often
are actually slower than the merge joins. In 21.1 release we will be
removing the corresponding code entirely.

Release note (sql change): Interleaved joins are now disabled by default
and will be entirely removed in 21.1 release because they are often
slower than the merge join.
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.

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


pkg/sql/logictest/testdata/logic_test/select_for_update_nowait_interleaved, line 10 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

It could be because we're switching users down below, which I believe means that we're creating a new session. Setting a session variable only impacts the current session, while setting the cluster setting impacts the initial value of the session variables for all sessions.

Oh, I didn't realize that switching the user starts a new session, makes sense, thanks.

@asubiotto
Copy link
Contributor

ok to merge this per latest guidance

@yuzefovich yuzefovich merged commit 4ecea82 into cockroachdb:release-20.2 Sep 18, 2020
@yuzefovich yuzefovich deleted the disable-ij branch September 18, 2020 19:46
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.

4 participants