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: fix panic on UPDATE RETURNING * in middle of schema change #32188

Merged
merged 1 commit into from Nov 26, 2018

Conversation

Projects
None yet
3 participants
@vivekmenezes
Copy link
Contributor

vivekmenezes commented Nov 8, 2018

fixes #32177

Release note (sql change): fix panic on UPDATE RETURNING * during
a schema change.

@vivekmenezes vivekmenezes requested a review from jordanlewis Nov 8, 2018

@vivekmenezes vivekmenezes requested review from cockroachdb/sql-execution-prs as code owners Nov 8, 2018

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

cockroach-teamcity commented Nov 8, 2018

This change is Reviewable

@vivekmenezes

This comment has been minimized.

Copy link
Contributor

vivekmenezes commented Nov 8, 2018

hold on a second. let me add some more tests

@vivekmenezes vivekmenezes force-pushed the vivekmenezes:returning branch from 1c7f6a2 to fe94894 Nov 8, 2018

@vivekmenezes

This comment has been minimized.

Copy link
Contributor

vivekmenezes commented Nov 8, 2018

@jordanlewis added more tests including tests that show case issue #29494 which I've reopened

@vivekmenezes

This comment has been minimized.

Copy link
Contributor

vivekmenezes commented Nov 8, 2018

@jordanlewis since the customer has a way forward without this fix, I'm going to work on getting this fix in without reopening #29494

Hold off on reviewing this.

@vivekmenezes vivekmenezes force-pushed the vivekmenezes:returning branch 2 times, most recently from 80d9176 to 313135f Nov 8, 2018

@vivekmenezes vivekmenezes changed the title sql: fix panic caused by UPDATE RETURNING * sql: fix panic on UPDATE RETURNING * in middle of schema change Nov 8, 2018

@vivekmenezes

This comment has been minimized.

Copy link
Contributor

vivekmenezes commented Nov 8, 2018

@jordanlewis @knz I was able to fix this without reverting the change. The fix turned out a lot simpler than I expected.

@vivekmenezes

This comment has been minimized.

Copy link
Contributor

vivekmenezes commented Nov 26, 2018

ping

@knz

knz approved these changes Nov 26, 2018

Copy link
Member

knz left a comment

nice!

// visible. We do not want them to be available for RETURNING.
//
// MakeUpdater guarantees that the first columns of the new values
// are those specified u.column .

This comment has been minimized.

@knz

knz Nov 26, 2018

Member

nit: "those specified in u.columns."

This comment has been minimized.

@vivekmenezes

vivekmenezes Nov 26, 2018

Contributor

Done

sql: fix panic on UPDATE RETURNING * in middle of schema change
fixes #32177

Release note (sql change): fix panic on UPDATE RETURNING * during
a schema change.

@vivekmenezes vivekmenezes force-pushed the vivekmenezes:returning branch from 313135f to 54a1ab4 Nov 26, 2018

@vivekmenezes

This comment has been minimized.

Copy link
Contributor

vivekmenezes commented Nov 26, 2018

bors r+

craig bot pushed a commit that referenced this pull request Nov 26, 2018

Merge #32188
32188: sql: fix panic on UPDATE RETURNING * in middle of schema change r=vivekmenezes a=vivekmenezes

fixes #32177

Release note (sql change): fix panic on UPDATE RETURNING * during
a schema change.

Co-authored-by: Vivek Menezes <vivek@cockroachlabs.com>
@craig

This comment has been minimized.

Copy link

craig bot commented Nov 26, 2018

Build succeeded

@craig craig bot merged commit 54a1ab4 into cockroachdb:master Nov 26, 2018

3 checks passed

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

@vivekmenezes vivekmenezes deleted the vivekmenezes:returning branch Nov 26, 2018

vivekmenezes added a commit to vivekmenezes/cockroach that referenced this pull request Jan 2, 2019

sql: stop returning dropped unique columns in DELETE RETURNING
this change is a followup to fix cockroachdb#32188

unique columns have an index on them. When such a column is dropped,
a DELETE statement will read a dropped column to correctly drop
the index on it, but this dropped column is exposed through the
RETURNING clause. This change stops exposing the column via RETURNING.

fixes cockroachdb#33361

Release note (sql change): fixed problem with returning dropped
unique columns in DELETE RETURNING statement.

craig bot pushed a commit that referenced this pull request Jan 3, 2019

Merge #33438
33438: sql: stop returning dropped unique columns in DELETE RETURNING r=vivekmenezes a=vivekmenezes

this change is a followup to fix #32188

unique columns have an index on them. When such a column is dropped,
a DELETE statement will read a dropped column to correctly drop
the index on it, but this dropped column is exposed through the
RETURNING clause. This change stops exposing the column via RETURNING.

fixes #33361

Release note (sql change): fixed problem with returning dropped
unique columns in DELETE RETURNING statement.

Co-authored-by: Vivek Menezes <vivek@cockroachlabs.com>

vivekmenezes added a commit to vivekmenezes/cockroach that referenced this pull request Jan 7, 2019

sql: stop returning dropped unique columns in DELETE RETURNING
this change is a followup to fix cockroachdb#32188

unique columns have an index on them. When such a column is dropped,
a DELETE statement will read a dropped column to correctly drop
the index on it, but this dropped column is exposed through the
RETURNING clause. This change stops exposing the column via RETURNING.

fixes cockroachdb#33361

Release note (sql change): fixed problem with returning dropped
unique columns in DELETE RETURNING statement.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment