/cockroach

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: enable client to override index selection for DELETE and UPDATE #31012

Merged
merged 1 commit into from Oct 12, 2018

Conversation

Reviewers

@BramGruneir BramGruneir

@RaduBerinde RaduBerinde

@cockroachdb/sql-rest-prs sql-rest-prs

@cockroachdb/sql-language-prs sql-language-prs

@cockroachdb/sql-execution-prs sql-execution-prs

@cockroachdb/sql-planning-prs sql-planning-prs

@jordanlewis jordanlewis

Assignees
No one assigned
Labels
Projects
  
Finished (milestone r2.1) in SQL Front-end, Lang & Semantics
Milestone
No milestone
5 participants
@knz @cockroach-teamcity @awoods187 @BramGruneir @RaduBerinde
@knz
Member

knz commented Oct 5, 2018

Informs #30734.

Prior to this patch, if the automatic index selection for DELETE or
UPDATE was inadequate, there was no way to override it like it is
possible for SELECT/INSERT/UPSERT.

This patch fixes this by extending and supporting the syntax for
DELETE and UPDATE:

DELETE FROM tbl@idx ...
UPDATE tbl@idx SET ...

Release note (sql change): it is now possible to force a specific
index for DELETE or UPDATE.

@knz knz requested review from jordanlewis, BramGruneir and RaduBerinde Oct 5, 2018

@knz knz added this to Triage in SQL Front-end, Lang & Semantics via automation Oct 5, 2018

@knz knz requested review from cockroachdb/sql-language-prs as code owners Oct 5, 2018

@cockroach-teamcity

This comment has been minimized.

Show comment
Hide comment
@cockroach-teamcity

cockroach-teamcity Oct 5, 2018

Member

This change is Reviewable
Member

cockroach-teamcity commented Oct 5, 2018

This change is Reviewable
@knz

This comment has been minimized.

Show comment
Hide comment
@knz

knz Oct 5, 2018

Member

@awoods187 this PR is meant to enable the workaround hinted in #30734 (comment)

This PR is also sufficiently simple that we can back-port it to both 2.0 and 2.1.

Can you check whether the customer finds this adequate?
Member

knz commented Oct 5, 2018

@awoods187 this PR is meant to enable the workaround hinted in #30734 (comment)

This PR is also sufficiently simple that we can back-port it to both 2.0 and 2.1.

Can you check whether the customer finds this adequate?

@knz knz added the docs-todo label Oct 5, 2018

@knz knz referenced this pull request Oct 5, 2018

Open

sql: a delete query uses the wrong index #30734

@awoods187

This comment has been minimized.

Show comment
Hide comment
@awoods187

awoods187 Oct 5, 2018

Contributor

Asked the customer and am awaiting the response
Contributor

awoods187 commented Oct 5, 2018

Asked the customer and am awaiting the response

@knz knz requested review from cockroachdb/sql-execution-prs as code owners Oct 5, 2018

@RaduBerinde

RaduBerinde approved these changes Oct 7, 2018
View changes

:lgtm:

Thanks for adding this workaround!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)

@knz knz moved this from Triage to Current milestone in SQL Front-end, Lang & Semantics Oct 8, 2018

@BramGruneir

BramGruneir approved these changes Oct 8, 2018
View changes

:lgtm:

Reviewed 8 of 8 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (and 1 stale)

@knz

This comment has been minimized.

Show comment
Hide comment
@knz

knz Oct 8, 2018

Member

Thanks for your reviews!

However I'll hold off merging this until we get a confirmation from the user that the approach is adequate for their needs.
Member

knz commented Oct 8, 2018

Thanks for your reviews!

However I'll hold off merging this until we get a confirmation from the user that the approach is adequate for their needs.
@BramGruneir

This comment has been minimized.

Show comment
Hide comment
@BramGruneir

BramGruneir Oct 8, 2018

Member

I still think you should merge this. It's sensible to allow overrides the the index selection.
Member

BramGruneir commented Oct 8, 2018

I still think you should merge this. It's sensible to allow overrides the the index selection.
@knz

This comment has been minimized.

Show comment
Hide comment
@knz

knz Oct 8, 2018

Member

Yeah but it's a feature extension, and so late in the release cycle it would not be appropriate for a gratuitous backport. Then:

  • if we don't backport it into 2.1, then I think this PR should not be merged as-is and instead we should wait for later 2.2 work on plan hints. Maybe the syntax designed for that roadmap item will be better.

  • to force a backport into 2.1 (and possibly 2.0) we need a strong customer case. This is what I am requesting.
Member

knz commented Oct 8, 2018

Yeah but it's a feature extension, and so late in the release cycle it would not be appropriate for a gratuitous backport. Then:

  • if we don't backport it into 2.1, then I think this PR should not be merged as-is and instead we should wait for later 2.2 work on plan hints. Maybe the syntax designed for that roadmap item will be better.

  • to force a backport into 2.1 (and possibly 2.0) we need a strong customer case. This is what I am requesting.
@awoods187

This comment has been minimized.

Show comment
Hide comment
@awoods187

awoods187 Oct 8, 2018

Contributor

While it would be nice to get customer confirmation, I think we should go ahead and do this. Even if we decide to implement a different hint syntax in the future this can still be valuable to customers now.
Contributor

awoods187 commented Oct 8, 2018

While it would be nice to get customer confirmation, I think we should go ahead and do this. Even if we decide to implement a different hint syntax in the future this can still be valuable to customers now.
@knz

This comment has been minimized.

Show comment
Hide comment
@knz

knz Oct 11, 2018

Member

All right, let's do this.

bors r+
Member

knz commented Oct 11, 2018

All right, let's do this.

bors r+

@knz knz referenced this pull request Oct 11, 2018

Merged

release-2.1: sql: enable client to override index selection for DELETE and UPDATE #31279

craig bot pushed a commit that referenced this pull request Oct 11, 2018

craig[bot] @knz
Merge #31012 
31012: sql: enable client to override index selection for DELETE and UPDATE r=knz a=knz

Informs #30734.

Prior to this patch, if the automatic index selection for DELETE or
UPDATE was inadequate, there was no way to override it like it is
possible for SELECT/INSERT/UPSERT.

This patch fixes this by extending and supporting the syntax for
DELETE and UPDATE:

```
DELETE FROM tbl@idx ...
UPDATE tbl@idx SET ...
```

Release note (sql change): it is now possible to force a specific
index for DELETE or UPDATE.

Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
Loading status checks…
e689878

@knz knz moved this from Current milestone to Finished (milestone r2.1) in SQL Front-end, Lang & Semantics Oct 11, 2018

@craig

This comment has been minimized.

Show comment
Hide comment
@craig

craig bot Oct 11, 2018

Build failed

craig bot commented Oct 11, 2018

Build failed
@knz
sql: enable client to override index selection for DELETE and UPDATE 
Prior to this patch, if the automatic index selection for DELETE or
UPDATE was inadequate, there was no way to override it like it is
possible for SELECT/INSERT/UPSERT.

This patch fixes this by extending and supporting the syntax for
DELETE and UPDATE:

```
DELETE FROM tbl@idx ...
UPDATE tbl@idx SET ...
```

Release note (sql change): it is now possible to force a specific
index for DELETE or UPDATE.
Loading status checks…
f0fe869
@knz

This comment has been minimized.

Show comment
Hide comment
@knz

knz Oct 12, 2018

Member

bors r+
Member

knz commented Oct 12, 2018

bors r+

craig bot pushed a commit that referenced this pull request Oct 12, 2018

craig[bot] @knz
Merge #31012 
31012: sql: enable client to override index selection for DELETE and UPDATE r=knz a=knz

Informs #30734.

Prior to this patch, if the automatic index selection for DELETE or
UPDATE was inadequate, there was no way to override it like it is
possible for SELECT/INSERT/UPSERT.

This patch fixes this by extending and supporting the syntax for
DELETE and UPDATE:

```
DELETE FROM tbl@idx ...
UPDATE tbl@idx SET ...
```

Release note (sql change): it is now possible to force a specific
index for DELETE or UPDATE.

Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
Loading status checks…
b7baa84
@craig

This comment has been minimized.

Show comment
Hide comment
@craig

craig bot Oct 12, 2018

Build succeeded

craig bot commented Oct 12, 2018

Build succeeded

@craig craig bot merged commit f0fe869 into cockroachdb:master Oct 12, 2018
3 checks passed

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@knz knz deleted the knz:20181005-custom-mutation-idx branch Oct 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment