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

[DocDB] Receiving a null range condition results in a crash in DocRowwiseIterator #12481

Closed
tanujnay112 opened this issue May 11, 2022 · 0 comments
Assignees
Labels
area/docdb YugabyteDB core features kind/bug This issue is a bug priority/medium Medium priority issue
Projects

Comments

@tanujnay112
Copy link
Contributor

tanujnay112 commented May 11, 2022

Jira Link: [DB-311](https://yugabyte.atlassian.net/browse/DB-311)

Description

It appears that receiving a condition involving NULL at the DocDB layer from YCQL or YSQL improperly causes a kTombstone value to be set as a scan bound for HybridScanChoices, causing a crash when we attempt to append this to a DocKey. The following is a repro of the issue at hand in YCQL:

CREATE KEYSPACE sample_key;
USE SAMPLE_KEY;
CREATE TABLE sample (
     h int,
     r1 int,
     r2 int,
     r3 int,
     PRIMARY KEY (h, r1)
 ) WITH CLUSTERING ORDER BY (r1 ASC)
     AND default_time_to_live = 0
     AND transactions = {'enabled': 'true'};
CREATE INDEX sample_index ON sample_key.sample (r1, r2, r3, h)
    WITH CLUSTERING ORDER BY (r2 ASC, r3 ASC, h asc)
    AND transactions = {'enabled': 'true'};
INSERT INTO sample(h,r1,r2,r3) VALUES (1,2,null,5);
SELECT * FROM sample WHERE r1 = 2 and r2 = NULL and r3 IN (5,55);

The final SELECT up above results in a tserver crash as we improperly convert the condition on r2 into a kTombstone value here and then we attempt to later append this to a DocKey during iteration causing a crash here.

@tanujnay112 tanujnay112 added area/docdb YugabyteDB core features status/awaiting-triage Issue awaiting triage labels May 11, 2022
@tanujnay112 tanujnay112 self-assigned this May 11, 2022
@yugabyte-ci yugabyte-ci added kind/bug This issue is a bug priority/medium Medium priority issue labels May 11, 2022
tanujnay112 added a commit that referenced this issue May 14, 2022
Summary:
Before this change, the scanspecs were indiscriminately using `FromQLValuePB` to convert from a `QLValuePB` to a `KeyEntryValue`/`PrimitiveValue`. This method converts nulls that were represented by a `QLValuePB` whose type is `VALUE_NOT_SET`. `FromQLValuePB` converts these objects into a `KeyEntryValue`/`PrimitiveValue` with type `kTombstone` which cannot be used in DocKeys. This later results in problems down the stack such as a crash in DocRowwiseIterator when it improperly tries to use the tombstone as a scan bound.

The proper thing to do when producing values for keys is to check whether or not the `QLValuePB` represents a null value and transform to `kNullLow`/`kNullHigh` accordingly. This change introduces a new method called `FromQLValuePBForKey` to encapsulate this logic.

There are numerous places in the code that either follow the pattern or should be following the pattern of checking for a `QLValuePB` being null before invoking `FromQLValuePB` on it. These should technically be using the new `FromQLValuePBForKey` method but this will be done in a later change.

Test Plan:
./yb_build.sh release --java-test org.yb.cql.TestIndexSelection
./yb_build.sh release --java-test org.yb.pgsql.TestPgRegressIndex

Reviewers: rsami, mihnea

Reviewed By: mihnea

Subscribers: bogdan, yql

Differential Revision: https://phabricator.dev.yugabyte.com/D16927
tanujnay112 added a commit that referenced this issue May 14, 2022
…specs

Summary:
Before this change, the scanspecs were indiscriminately using `FromQLValuePB` to convert from a `QLValuePB` to a `KeyEntryValue`/`PrimitiveValue`. This method converts nulls that were represented by a `QLValuePB` whose type is `VALUE_NOT_SET`. `FromQLValuePB` converts these objects into a `KeyEntryValue`/`PrimitiveValue` with type `kTombstone` which cannot be used in DocKeys. This later results in problems down the stack such as a crash in DocRowwiseIterator when it improperly tries to use the tombstone as a scan bound.

The proper thing to do when producing values for keys is to check whether or not the `QLValuePB` represents a null value and transform to `kNullLow`/`kNullHigh` accordingly. This change introduces a new method called `FromQLValuePBForKey` to encapsulate this logic.

There are numerous places in the code that either follow the pattern or should be following the pattern of checking for a `QLValuePB` being null before invoking `FromQLValuePB` on it. These should technically be using the new `FromQLValuePBForKey` method but this will be done in a later change.

Original commit: 274784f22fe485ce03cf07c05aa61b700d2ce934
Original revision: D16927

Test Plan:
./yb_build.sh release --java-test org.yb.cql.TestIndexSelection
./yb_build.sh release --java-test org.yb.pgsql.TestPgRegressIndex

Reviewers: rsami, mihnea

Reviewed By: mihnea

Subscribers: yql, bogdan

Differential Revision: https://phabricator.dev.yugabyte.com/D16990
tanujnay112 added a commit that referenced this issue May 14, 2022
…pecs

Summary:
Before this change, the scanspecs were indiscriminately using `FromQLValuePB` to convert from a `QLValuePB` to a `KeyEntryValue`/`PrimitiveValue`. This method converts nulls that were represented by a `QLValuePB` whose type is `VALUE_NOT_SET`. `FromQLValuePB` converts these objects into a `KeyEntryValue`/`PrimitiveValue` with type `kTombstone` which cannot be used in DocKeys. This later results in problems down the stack such as a crash in DocRowwiseIterator when it improperly tries to use the tombstone as a scan bound.

The proper thing to do when producing values for keys is to check whether or not the `QLValuePB` represents a null value and transform to `kNullLow`/`kNullHigh` accordingly. This change introduces a new method called `FromQLValuePBForKey` to encapsulate this logic.

There are numerous places in the code that either follow the pattern or should be following the pattern of checking for a `QLValuePB` being null before invoking `FromQLValuePB` on it. These should technically be using the new `FromQLValuePBForKey` method but this will be done in a later change.

Original commit: 274784f22fe485ce03cf07c05aa61b700d2ce934
Original revision: D16927

Test Plan:
Jenkins: rebase: 2.8
./yb_build.sh release --java-test org.yb.cql.TestIndexSelection
./yb_build.sh release --java-test org.yb.pgsql.TestPgRegressIndex

Reviewers: rsami, mihnea

Reviewed By: mihnea

Subscribers: yql, bogdan

Differential Revision: https://phabricator.dev.yugabyte.com/D16989
@sushantrmishra sushantrmishra added this to Backlog in YSQL via automation Jun 5, 2022
@yugabyte-ci yugabyte-ci removed the status/awaiting-triage Issue awaiting triage label Jun 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docdb YugabyteDB core features kind/bug This issue is a bug priority/medium Medium priority issue
Projects
YSQL
  
Backlog
Development

No branches or pull requests

2 participants