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-22.2: xform: avoid locality-optimized scans which must always read remote rows #87866

Merged
merged 1 commit into from
Sep 13, 2022

Conversation

blathers-crl[bot]
Copy link

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

Backport 1/1 commits from #87350 on behalf of @msirek.

/cc @cockroachdb/release


Previously, for tables with old-style partitioning, which don't use the
new multiregion abstractions, there were no guardrails in place to
prevent 2 cases where locality-optimized scan must always read ranges in
remote regions:

  1. When a scan with no hard limit has a non-unique index constraint
    (could return more than one row per matched index key, not including
    the partitioning key column)
  2. When the max cardinality of a constrained scan is less than the hard
    limit placed on the scan via a LIMIT clause

This was inadequate because locality-optimized scan is usually slower
than distributed scans when reading from remote regions is required. If
we can statically determine reading from remote regions is required,
locality-optimized scan should not even be costed and considered by the
optimizer. Multiregion tables, such as REGIONAL BY ROW tables, don't
encounter this issue because the internal crdb_region partitioning
column is not part of the UNIQUE constraint in that case, for example:

CREATE TABLE regional_by_row_table (
  col1 int,
  col2 bool NOT NULL,
  UNIQUE INDEX idx(col1) -- crdb_region is implicitly the 1st idx col
) LOCALITY REGIONAL BY ROW;

SELECT * FROM regional_by_row_table WHERE col1 = 1;

In the above, we could use LOS and split this into a local scan:
SELECT * FROM regional_by_row_table WHERE crdb_region = 'us' AND col1 = 1;
... and remote scans:

SELECT * FROM regional_by_row_table WHERE crdb_region IN ('ca', 'ap')
          AND col1 = 1;

The max cardinality of the local scan is 1, and the max cardinality of
the original scan is 1, so we know it's possible to fulfill the request
solely with the local scan.

To address this, this patch avoids planning locality-optimized scan for
the two cases listed at the top of the description. The first case is
detected by the local scan of the UNION ALL having a lower max
cardinality than the max cardinality including all constraint spans
(for example, given a pair of columns (part_col, col1), if col1 is a
unique key, then max_cardinality(col1) will equal
max_cardinality(part_col, col1). The second case is detected by a
direct comparison of the hard limit with the max cardinality of the
local scan.

Release justification: Low risk fix for suboptimal locality-optimized scan

Release note (bug fix): Because of a misused query optimization involving tables with one or more PARTITION BY clauses and partition zone constraints which assign region locality to those partitions, in some cases the optimizer would pick a locality-optimized search query plan which is not truly locality-optimized and has higher latency than competing query plans which use distributed scan. Locality-optimized search is now avoided in cases which are known not to benefit from this optimization.

Previously, for tables with old-style partitioning, which don't use the
new multiregion abstractions, there were no guardrails in place to
prevent 2 cases where locality-optimized scan must always read ranges in
remote regions:

1. When a scan with no hard limit has a non-unique index constraint
   (could return more than one row per matched index key, not including
   the partitioning key column)
2. When the max cardinality of a constrained scan is less than the hard
   limit placed on the scan via a LIMIT clause

This was inadequate because locality-optimized scan is usually slower
than distributed scans when reading from remote regions is required. If
we can statically determine reading from remote regions is required,
locality-optimized scan should not even be costed and considered by the
optimizer. Multiregion tables, such as REGIONAL BY ROW tables, don't
encounter this issue because the internal `crdb_region` partitioning
column is not part of the UNIQUE constraint in that case, for example:
```
CREATE TABLE regional_by_row_table (
  col1 int,
  col2 bool NOT NULL,
  UNIQUE INDEX idx(col1) -- crdb_region is implicitly the 1st idx col
) LOCALITY REGIONAL BY ROW;

SELECT * FROM regional_by_row_table WHERE col1 = 1;
```
In the above, we could use LOS and split this into a local scan:
`SELECT * FROM regional_by_row_table WHERE crdb_region = 'us' AND col1 = 1;`
... and remote scans:
```
SELECT * FROM regional_by_row_table WHERE crdb_region IN ('ca', 'ap')
          AND col1 = 1;
```
The max cardinality of the local scan is 1, and the max cardinality of
the original scan is 1, so we know it's possible to fulfill the request
solely with the local scan.

To address this, this patch avoids planning locality-optimized scan for
the two cases listed at the top of the description. The first case is
detected by the local scan of the UNION ALL having a lower max
cardinality than the max cardinality including all constraint spans
(for example, given a pair of columns (part_col, col1), if col1 is a
unique key, then max_cardinality(col1) will equal
max_cardinality(part_col, col1). The second case is detected by a
direct comparison of the hard limit with the max cardinality of the
local scan.

Release note (bug fix): This patch fixes a misused query optimization
involving tables with one or more PARTITION BY clauses and partition
zone constraints which assign region locality to those partitions.
In some cases the optimizer picks a `locality-optimized search` query
plan which is not truly locality-optimized, and has higher latency than
competing query plans which use distributed scan. Locality-optimized
search is now avoided in cases which are known not to benefit from this
optimization.

Release justification: Low risk fix for suboptimal locality-optimized scan
@blathers-crl blathers-crl bot requested a review from a team as a code owner September 12, 2022 19:32
@blathers-crl blathers-crl bot force-pushed the blathers/backport-release-22.2-87350 branch from 2cb2a7a to c9c5956 Compare September 12, 2022 19:32
@blathers-crl blathers-crl bot added blathers-backport This is a backport that Blathers created automatically. O-robot Originated from a bot. labels Sep 12, 2022
@blathers-crl
Copy link
Author

blathers-crl bot commented Sep 12, 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?

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@msirek msirek left a comment

Choose a reason for hiding this comment

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

TFTR!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball and @yuzefovich)

@msirek msirek merged commit 030850c into release-22.2 Sep 13, 2022
@msirek msirek deleted the blathers/backport-release-22.2-87350 branch September 13, 2022 16:00
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