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

opt: use anti-join for INSERT ON CONFLICT DO NOTHING conflict detection #58679

Merged

Conversation

mgartner
Copy link
Collaborator

@mgartner mgartner commented Jan 8, 2021

opt: use anti-join for INSERT ON CONFLICT DO NOTHING conflict detection

The addition of lookup anti-joins enables the usage of anti-joins,
instead of left joins and filters, for detecting conflicts in an
INSERT ... ON CONFLICT ... DO NOTHING statement. This simplifies the
query plans for these statements.

As a result of this change two normalization rules had to be updated:

  1. EliminateGroupByProject no longer matches UpsertDistinctOn
    expressions because they no longer are built with a Project as
    a child.
  2. The AreValuesDistinct helper function for
    EliminateDistinctOnValues now detects when the left child of an
    AntiJoin has distinct values. This is required for eliminating
    UpsertDistinctOn unnecessary operators now that their inputs are
    AntiJoins.

Release note (performance improvement): INSERT ... ON CONFLICT ... DO
NOTHING statements now use anti-joins for detecting conflicts. This
simplifies the query plan for these statements, which may result in more
efficient execution.

opt: replace &memo.JoinPrivate{} with memo.EmptyJoinPrivate

Release note: None

@mgartner mgartner requested a review from a team as a code owner January 8, 2021 23:53
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

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

:lgtm: Nice!

Reviewed 10 of 10 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball, @mgartner, and @RaduBerinde)


pkg/sql/opt/norm/rules/groupby.opt, line 18 at r1 (raw file):

# Note: EliminateGroupByProject should be located above
# EliminateJoinUnderGroupByLeft so that it can remove any interfering Projects.
[EliminateGroupByProject, Normalize]

[nit] maybe update the comment to explain why UpsertDistinctOn is not included.


pkg/sql/opt/optbuilder/insert.go, line 767 at r1 (raw file):

					fetchScope.expr,
					on,
					memo.EmptyJoinPrivate,

is there a reason you don't want to use memo.EmptyJoinPrivate?

@mgartner mgartner force-pushed the use-anti-join-for-insert-conflict-do-nothing branch from 89f0de4 to 796973c Compare January 11, 2021 18:20
Copy link
Collaborator Author

@mgartner mgartner 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 (and 1 stale) (waiting on @andy-kimball, @RaduBerinde, and @rytaft)


pkg/sql/opt/norm/rules/groupby.opt, line 18 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

[nit] maybe update the comment to explain why UpsertDistinctOn is not included.

Done.


pkg/sql/opt/optbuilder/insert.go, line 767 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

is there a reason you don't want to use memo.EmptyJoinPrivate?

Nope, just copied from some other places. I added a new commit that converts all other &memo.JoinPrivate{}s to memo.EmptyJoinPrivate.

Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r2, 2 of 2 files at r3.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball and @RaduBerinde)

@mgartner
Copy link
Collaborator Author

TFTR!

bors r=rytaft

@craig
Copy link
Contributor

craig bot commented Jan 11, 2021

Merge conflict.

The addition of lookup anti-joins enables the usage of anti-joins,
instead of left joins and filters, for detecting conflicts in an
`INSERT ... ON CONFLICT ... DO NOTHING` statement. This simplifies the
query plans for these statements.

As a result of this change two normalization rules had to be updated:

  1. `EliminateGroupByProject` no longer matches `UpsertDistinctOn`
     expressions because they no longer are built with a `Project` as
     a child.
  2. The `AreValuesDistinct` helper function for
     `EliminateDistinctOnValues` now detects when the left child of an
     AntiJoin has distinct values. This is required for eliminating
     `UpsertDistinctOn` unnecessary operators now that their inputs are
     AntiJoins.

Release note (performance improvement): INSERT ... ON CONFLICT ... DO
NOTHING statements now use anti-joins for detecting conflicts. This
simplifies the query plan for these statements, which may result in more
efficient execution.
@mgartner mgartner force-pushed the use-anti-join-for-insert-conflict-do-nothing branch from 796973c to 05a9be1 Compare January 12, 2021 01:19
@mgartner
Copy link
Collaborator Author

bors r=rytaft

@craig
Copy link
Contributor

craig bot commented Jan 12, 2021

Build succeeded:

@craig craig bot merged commit ee5c4e6 into cockroachdb:master Jan 12, 2021
@mgartner mgartner deleted the use-anti-join-for-insert-conflict-do-nothing branch January 12, 2021 19:15
craig bot pushed a commit that referenced this pull request Mar 23, 2021
62209: opt: add a regression test for INSERT ON CONFLICT with ORDER BY r=RaduBerinde a=RaduBerinde

Informs #57434.

Release note: None

@mgartner apparently you fixed this bug with #58679 :)

Co-authored-by: Radu Berinde <radu@cockroachlabs.com>
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.

None yet

3 participants