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

release-21.2: rowenc: fix needed column families computation for secondary indexes #88207

Merged
merged 1 commit into from
Sep 21, 2022

Conversation

blathers-crl[bot]
Copy link

@blathers-crl blathers-crl bot commented Sep 20, 2022

Backport 1/1 commits from #88174 on behalf of @yuzefovich.

/cc @cockroachdb/release


Previously, when determining the "minimal set of column families" required to retrieve all of the needed columns for the scan operation we could incorrectly not include the special zeroth family into the set. The KV for the zeroth column family is always present, so it might need to be fetched even when it's not explicitly needed when the "needed" column families are all nullable. Before this patch the code for determining whether all of the needed column families are nullable incorrectly assumed that all columns in a family are stored, but this is only true for the primary indexes - for the secondary indexes only those columns mentioned in STORING clause are actually stored (apart from the indexed and PK columns). As a result we could incorrectly not fetch a row if:

  • the unique secondary index is used
  • the needed column has a NULL value
  • all non-nullable columns from the same column family as the needed column are not stored in the index
  • other column families are not fetched.

This is now fixed by considering only the set of stored columns.

The bug seems relatively minor since it requires a multitude of conditions to be met, so I don't think it's a TA worthy.

Fixes: #88110.

Release note (bug fix): Previously, CockroachDB could incorrectly not fetch rows with NULL values when reading from the unique secondary index when multiple column families are defined for the table and the index doesn't store some of the NOT NULL columns.


Release justification: bug fix.

Previously, when determining the "minimal set of column families"
required to retrieve all of the needed columns for the scan operation we
could incorrectly not include the special zeroth family into the set.
The KV for the zeroth column family is always present, so it might need
to be fetched even when it's not explicitly needed when the "needed"
column families are all nullable. Before this patch the code for
determining whether all of the needed column families are nullable
incorrectly assumed that all columns in a family are stored, but this is
only true for the primary indexes - for the secondary indexes only those
columns mentioned in `STORING` clause are actually stored (apart from
the indexed and PK columns). As a result we could incorrectly not fetch
a row if:
- the unique secondary index is used
- the needed column has a NULL value
- all non-nullable columns from the same column family as the needed
column are not stored in the index
- other column families are not fetched.

This is now fixed by considering only the set of stored columns.

The bug seems relatively minor since it requires a multitude of
conditions to be met, so I don't think it's a TA worthy.

Release note (bug fix): Previously, CockroachDB could incorrectly not
fetch rows with NULL values when reading from the unique secondary index
when multiple column families are defined for the table and the index
doesn't store some of the NOT NULL columns.
@blathers-crl blathers-crl bot force-pushed the blathers/backport-release-21.2-88174 branch from 4ba304c to 8bd0a59 Compare September 20, 2022 02:52
@blathers-crl
Copy link
Author

blathers-crl bot commented Sep 20, 2022

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Patches should only be created for serious issues or test-only changes.
  • Patches should not break backwards-compatibility.
  • Patches should change as little code as possible.
  • Patches should not change on-disk formats or node communication protocols.
  • Patches should not add new functionality.
  • Patches must not add, edit, or otherwise modify cluster versions; or add version gates.
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

  • What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
  • Will this work in a cluster of mixed patch versions? Did we test that?
  • If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

@blathers-crl blathers-crl bot added blathers-backport This is a backport that Blathers created automatically. O-robot Originated from a bot. labels Sep 20, 2022
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@cucaroach cucaroach left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @yuzefovich)

@yuzefovich yuzefovich merged commit 912805d into release-21.2 Sep 21, 2022
@yuzefovich yuzefovich deleted the blathers/backport-release-21.2-88174 branch September 21, 2022 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blathers-backport This is a backport that Blathers created automatically. O-robot Originated from a bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants