-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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-24.3: opt/execbuilder: pick correct canary column ordinal #141823
release-24.3: opt/execbuilder: pick correct canary column ordinal #141823
Conversation
The execbuilder previously used the `colOrdMap` of an Upsert's input to find the ordinal of the canary column. In rare cases, the canary column could also be used as a partial index DEL column. Because a `colOrdMap` can only map each column ID to a single ordinal, and because partial index columns are positioned after the canary column, the ordinal fetched from the `colOrdMap` would be the partial index DEL column ordinal, not the canary column ordinal. This could cause node-crashing panics during execution when trying to access the canary canary column in the input row after the partial index columns are sliced off. As an example, consider the table and mutation: CREATE TABLE t (c BOOL PRIMARY KEY, INDEX (c) WHERE c) UPSERT INTO t VALUES (false) The input to the Upsert operator produces 4 physical columns: 1. An insert column with the value to insert if a row with the same PK does not already exist. We'll call this logical column `new_c`. 2. A canary column which is non-NULL if a row with the same PK already exists. This is always a PK column fetched from the table—in our example this is `c`. Let's call this logical column `old_c`. 3. A partial index PUT column which is true if the new row should be added to the partial index. Since the partial index predicate expression is the PK column, this is the same logical column as `new_c`. 4. A partial index DEL column which is true if the old row should be removed from the partial index. This is the same logical column as `old_c`. So, the physical columns produced by the input of the Upsert are: | insert column | canary column | partial index PUT | partial index DEL | ------------- | ------------- | ----------------- | ----------------- | `new_c` | `old_c` | `new_c` | `old_c` Using the `colOrdMap` to find the ordinal of `old_c` yields `3`, instead of `1`. During execution the partial index columns are sliced off, and the row becomes: | insert column | canary column | | ------------- | ------------- | | `new_c` | `old_c` | When the execution engine attempts to access index `3` of this row, the process panics with an out-of-bounds exception. This has been fixed by using the lengths of the insert (and other) columns to find the canary column ordinal, instead of the `colOrdMap`. Fixes cockroachdb#136458 Release note (bug fix): A bug has been fixed that could cause gateway nodes to panic when performing an `UPSERT` on a table with a `BOOL` primary key column and a partial index with the PK column as the predicate expression.
Thanks for opening a backport. Please check the backport criteria before merging:
If your backport adds new functionality, please ensure that the following additional criteria are satisfied:
Also, please add a brief release justification to the body of your PR to justify this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @mw5h)
Backport 1/2 commits from #141728.
/cc @cockroachdb/release
opt/execbuilder: pick correct canary column ordinal
The execbuilder previously used the
colOrdMap
of an Upsert's input tofind the ordinal of the canary column. In rare cases, the canary column
could also be used as a partial index DEL column. Because a
colOrdMap
can only map each column ID to a single ordinal, and because partial
index columns are positioned after the canary column, the ordinal
fetched from the
colOrdMap
would be the partial index DEL columnordinal, not the canary column ordinal.
This could cause node-crashing panics during execution when trying to
access the canary canary column in the input row after the partial index
columns are sliced off.
As an example, consider the table and mutation:
The input to the Upsert operator produces 4 physical columns:
PK does not already exist. We'll call this logical column
new_c
.exists. This is always a PK column fetched from the table—in our
example this is
c
. Let's call this logical columnold_c
.added to the partial index. Since the partial index predicate
expression is the PK column, this is the same logical column as
new_c
.removed from the partial index. This is the same logical column as
old_c
.So, the physical columns produced by the input of the Upsert are:
new_c
old_c
new_c
old_c
Using the
colOrdMap
to find the ordinal ofold_c
yields3
, insteadof
1
.During execution the partial index columns are sliced off, and the row
becomes:
new_c
old_c
When the execution engine attempts to access index
3
of this row, theprocess panics with an out-of-bounds exception.
This has been fixed by using the lengths of the insert (and other)
columns to find the canary column ordinal, instead of the
colOrdMap
.Fixes #136458
Release note (bug fix): A bug has been fixed that could cause gateway
nodes to panic when performing an
UPSERT
on a table with aBOOL
primary key column and a partial index with the PK column as the
predicate expression.
Release justification: Low-risk bug fix.