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

opt: plan lookup joins on indexes with virtual columns in more cases #75872

Closed
mgartner opened this issue Feb 2, 2022 · 1 comment · Fixed by #76078
Closed

opt: plan lookup joins on indexes with virtual columns in more cases #75872

mgartner opened this issue Feb 2, 2022 · 1 comment · Fixed by #76078
Assignees
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-queries SQL Queries Team

Comments

@mgartner
Copy link
Collaborator

mgartner commented Feb 2, 2022

Currently, GenerateLookupJoinsWithVirtualCols can only plan a lookup join on an index with a virtual column when there is a projected column with the same column ID as the virtual column ID in the table metadata. This is overly restrictive, preventing optimal plans in many cases.

For example, consider the virtual column created for an expression index:

exec-ddl
CREATE TABLE small (
    k INT PRIMARY KEY,
    a INT
);
----

exec-ddl
CREATE TABLE virt (
    k INT PRIMARY KEY,
    a INT,
    INDEX ((a * 100))
);
----

norm
SELECT * FROM small INNER LOOKUP JOIN virt ON small.a = virt.a * 100
----
project
 ├── columns: k:1!null a:2!null b:3 k:6!null a:7 b:8
 ├── immutable
 ├── key: (1,6)
 ├── fd: (1)-->(2,3), (6)-->(7,8), (7)-->(2)
 └── inner-join (hash)
      ├── columns: small.k:1!null small.a:2!null small.b:3 virt.k:6!null virt.a:7 virt.b:8 column12:12!null
      ├── flags: force lookup join (into right side)
      ├── immutable
      ├── key: (1,6)
      ├── fd: (1)-->(2,3), (6)-->(7,8), (7)-->(12), (2)==(12), (12)==(2)
      ├── scan small
      │    ├── columns: small.k:1!null small.a:2 small.b:3
      │    ├── key: (1)
      │    └── fd: (1)-->(2,3)
      ├── project
      │    ├── columns: column12:12 virt.k:6!null virt.a:7 virt.b:8
      │    ├── immutable
      │    ├── key: (6)
      │    ├── fd: (6)-->(7,8), (7)-->(12)
      │    ├── scan virt
      │    │    ├── columns: virt.k:6!null virt.a:7 virt.b:8
      │    │    ├── computed column expressions
      │    │    │    └── crdb_internal_idx_expr:11
      │    │    │         └── virt.a:7 * 100
      │    │    ├── key: (6)
      │    │    └── fd: (6)-->(7,8)
      │    └── projections
      │         └── virt.a:7 * 100 [as=column12:12, outer=(7), immutable]
      └── filters
           └── small.a:2 = column12:12 [outer=(2,12), constraints=(/2: (/NULL - ]; /12: (/NULL - ]), fd=(2)==(12), (12)==(2)]

Notice that in the canonical expression, the projection of the virtual column expression is column12:12, while the virtual computed column in virt's table metadat is crdb_internal_idx_expr:11. Because these are not the same column, a lookup join is not planned.

@mgartner mgartner added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label Feb 2, 2022
@mgartner mgartner self-assigned this Feb 2, 2022
@blathers-crl blathers-crl bot added the T-sql-queries SQL Queries Team label Feb 2, 2022
@mgartner
Copy link
Collaborator Author

mgartner commented Feb 2, 2022

Another example of a query where a lookup join could be planned but is not:

exec-ddl
CREATE TABLE small (
    k INT PRIMARY KEY,
    a INT,
    b INT
);
----

exec-ddl
CREATE TABLE virt (
    k INT PRIMARY KEY,
    a INT,
    b INT,
    v INT AS (a * 100) VIRTUAL,
    INDEX (v),
    INDEX (v, b)
);
----

opt
SELECT * FROM small INNER LOOKUP JOIN virt ON small.a = virt.a * 100
----
project
 ├── columns: k:1!null a:2!null b:3 k:6!null a:7 b:8 v:9
 ├── immutable
 ├── key: (1,6)
 ├── fd: (1)-->(2,3), (6)-->(7,8), (7)-->(2,9)
 └── inner-join (hash)
      ├── columns: small.k:1!null small.a:2!null small.b:3 virt.k:6!null virt.a:7 virt.b:8 v:9 column12:12!null
      ├── flags: force lookup join (into right side)
      ├── immutable
      ├── key: (1,6)
      ├── fd: (1)-->(2,3), (6)-->(7,8), (7)-->(9,12), (2)==(12), (12)==(2)
      ├── scan small
      │    ├── columns: small.k:1!null small.a:2 small.b:3
      │    ├── key: (1)
      │    └── fd: (1)-->(2,3)
      ├── project
      │    ├── columns: column12:12 v:9 virt.k:6!null virt.a:7 virt.b:8
      │    ├── immutable
      │    ├── key: (6)
      │    ├── fd: (6)-->(7,8), (7)-->(9,12)
      │    ├── scan virt
      │    │    ├── columns: virt.k:6!null virt.a:7 virt.b:8
      │    │    ├── computed column expressions
      │    │    │    └── v:9
      │    │    │         └── virt.a:7 * 100
      │    │    ├── key: (6)
      │    │    └── fd: (6)-->(7,8)
      │    └── projections
      │         ├── virt.a:7 * 100 [as=column12:12, outer=(7), immutable]
      │         └── virt.a:7 * 100 [as=v:9, outer=(7), immutable]
      └── filters
           └── small.a:2 = column12:12 [outer=(2,12), constraints=(/2: (/NULL - ]; /12: (/NULL - ]), fd=(2)==(12), (12)==(2)]

mgartner added a commit to mgartner/cockroach that referenced this issue Feb 4, 2022
ExtractJoinEqualities now reuses computed columns instead of
synthesizing new columns when it creates projections that exactly match
a computed column expression of a base table. This allows
GenerateLookupJoinsWithVirtualCols to generate lookup joins in more
cases. This also paves the way for exploring  anti- and semi-lookup
joins on indexes with virtual columns and expression indexes.

Fixes cockroachdb#75872

Release note (performance improvement): The optimizer now plans inner
lookup joins using expression indexes in more cases, resulting in more
efficient query plans.
craig bot pushed a commit that referenced this issue Feb 8, 2022
…76205

75541: rfc: kv lock observability via `crdb_locks` r=AlexTalks a=AlexTalks

_Link to [RFC text](https://github.com/AlexTalks/cockroach/blob/rfc_crdb_locks/docs/RFCS/20220104_crdb_locks.md)._

This RFC proposes a feature for observing locks (and contention on
locks) via a new virtual table called `crdb_internal.crdb_locks`.

Release note: None

75738: roachtest/follower_reads: unskip follower-reads/mixed-version/single-region r=andreimatei a=andreimatei

It should have been unskipped with the rest of the follower reads tests
when #69817 was closed.

Release note: None

75898: sql: consider virtual PK columns as stored in the optimizer r=mgartner a=mgartner

Allowing virtual primary key columns created a contradiction of two
hard-coded assumptions of the optimizer:

  1. Primary key columns are written to the primary index (and all
     secondary indexes).
  2. Virtual columns are not written to the primary index.

To prevent this contradiction from causing bugs while planning, the opt
catalog presents columns that are marked as virtual PK columns in the
descriptor as stored columns to the optimizer. This is valid because
virtual computed PK columns behave exactly like stored computed PK
columns: they are written to all indexes.

The test catalog has been updated to mimic this behavior.

Fixes #75147
Fixes #75874

Release note (bug fix): A bug has been fixed that caused internal errors
when querying tables with virtual columns in the primary key. This bug
was only present since version 22.1.0-alpha.1 and does not appear in any
production releases.

75912: util/tracing: don't return recordings for non-recording spans r=andreimatei a=andreimatei

This patch makes sp.GetRecording() return nil if the span was not
recording. This is already what the documentation on GetRecording() was
implying, but the actual behavior was different: a non-recording span
(that's not a no-op span) would return a non-nil recording consisting
only of the the span's metadata. This behavior is not necessarily
unreasonable, but it turns out it's a bit dangerous from a performance
perspective - there's a cost (e.g. some allocations) for generating this
recording, and it was too easy for unsuspecting callers to pay that when
they didn't want to. In particular, DistSQL was indiscriminently calling
GetRecording() (through its GetTraceData* utilities), and paying this
measurable cost unintentionally.

Eliminating this cost is particularly important when the Tracer is
configured in the "active spans registry" tracing mode; when not in that
mode, all the spans would be no-ops and the GetRecording calls were
shortcircuited anyway. This is seen in BenchmarkTracing//trace=on,
particularly on "3node/scan" which was the benchmark with the bigest
difference between tracing on and off.

name                              old time/op    new time/op    delta
Tracing/1node/scan/trace=on-32       214µs ± 4%     206µs ± 5%    ~     (p=0.222 n=5+5)
Tracing/1node/insert/trace=on-32     338µs ± 5%     334µs ± 4%    ~     (p=0.690 n=5+5)
Tracing/3node/scan/trace=on-32       515µs ± 3%     484µs ± 3%  -6.11%  (p=0.008 n=5+5)
Tracing/3node/insert/trace=on-32     635µs ± 1%     627µs ± 1%    ~     (p=0.095 n=5+5)

name                              old alloc/op   new alloc/op   delta
Tracing/1node/scan/trace=on-32      27.2kB ± 2%    25.6kB ± 2%  -5.94%  (p=0.008 n=5+5)
Tracing/1node/insert/trace=on-32    45.2kB ± 2%    45.5kB ± 2%    ~     (p=0.690 n=5+5)
Tracing/3node/scan/trace=on-32      83.2kB ± 3%    79.7kB ± 5%    ~     (p=0.095 n=5+5)
Tracing/3node/insert/trace=on-32     102kB ± 2%     103kB ± 1%    ~     (p=0.548 n=5+5)

name                              old allocs/op  new allocs/op  delta
Tracing/1node/scan/trace=on-32         242 ± 1%       229 ± 0%  -5.37%  (p=0.000 n=5+4)
Tracing/1node/insert/trace=on-32       338 ± 1%       339 ± 1%    ~     (p=0.563 n=5+5)
Tracing/3node/scan/trace=on-32         819 ± 1%       788 ± 6%    ~     (p=0.135 n=5+5)
Tracing/3node/insert/trace=on-32       871 ± 0%       880 ± 0%  +0.99%  (p=0.016 n=4+5)

Release note: None

75981: kvserver: clean-up the interaction between the mergeQueue and `AdminRelocateRange` r=aayushshah15 a=aayushshah15

This commit makes it such that the `mergeQueue` no longer has to take special
care to avoid a lease transfer when calling into `AdminRelocateRange`. This was
enabled by #75676.

Release note: None


75999: kvserver: stop reconciling lease count imbalances in the `StoreRebalancer` r=aayushshah15 a=aayushshah15

Generally, the `StoreRebalancer` doesn't try to attempt lease transfers for
replicas that account for less than 0.1% of the store's total QPS. However, it
allows such lease transfers if the store has a higher than average number of
leases. This is fraught because of the following reasons:

1. It doesn't adhere to any padding. It will do these tiny lease transfers
until the store has less than or equal to the average number of leases.
2. Additionally, "average" here means the average across the cluster, not just
the stores that have replicas for the range. This is clearly not good, as this
doesn't translate well to heterogeneously loaded clusters.
3. These lease transfers also don't adhere to
`kv.allocator.min_lease_transfer_interval`. This means they are not rate
limited like the lease transfers done by the replicateQueue.

This patch fixes this behavior. This patch is similar to #64559.

Noticed while running a tpcc workload on a multi-region cluster, but on a
database constrained to a single region. This was done to simulate a
"heterogeneously loaded cluster".

Release note: None


76078: opt: plan inner lookup joins on virtual column indexes in more cases r=mgartner a=mgartner

ExtractJoinEqualities now reuses computed columns instead of
synthesizing new columns when it creates projections that exactly match
a computed column expression of a base table. This allows
GenerateLookupJoinsWithVirtualCols to generate lookup joins in more
cases. This also paves the way for exploring  anti- and semi-lookup
joins on indexes with virtual columns and expression indexes.

Fixes #75872

Release note (performance improvement): The optimizer now plans inner
lookup joins using expression indexes in more cases, resulting in more
efficient query plans.

76193: optbuilder: do not create invalid casts when building CASE expressions r=mgartner a=mgartner

The optbuilder no longer creates invalid casts when building CASE
expressions that have branches of different types. CASE expressions that
previously caused internal errors now result in user-facing errors. A
similar change was made recently to UNION expressions in #75219.

Note that there is more work to be done to be fully consistent with
Postgres's CASE typing behavior, see #75365.

Fixes #75365

Release note (bug fix): CASE expressions with branches that result in
types that cannot be cast to a common type now result in a user-facing
error instead of an internal error.

76200: cloud: bump orchestrator to v21.2.5 r=celiala a=Azhng

Release note: None

76205: roachtest: update 22.1 version map to v21.2.5 r=celiala a=Azhng

Release note: None

Co-authored-by: Alex Sarkesian <[email protected]>
Co-authored-by: Andrei Matei <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: Aayush Shah <[email protected]>
Co-authored-by: Azhng <[email protected]>
@craig craig bot closed this as completed in 82b6db4 Feb 8, 2022
RajivTS pushed a commit to RajivTS/cockroach that referenced this issue Mar 6, 2022
ExtractJoinEqualities now reuses computed columns instead of
synthesizing new columns when it creates projections that exactly match
a computed column expression of a base table. This allows
GenerateLookupJoinsWithVirtualCols to generate lookup joins in more
cases. This also paves the way for exploring  anti- and semi-lookup
joins on indexes with virtual columns and expression indexes.

Fixes cockroachdb#75872

Release note (performance improvement): The optimizer now plans inner
lookup joins using expression indexes in more cases, resulting in more
efficient query plans.
@mgartner mgartner moved this to Done in SQL Queries Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-queries SQL Queries Team
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant