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: join reordering can produce incorrect query plans #76522

Closed
mgartner opened this issue Feb 14, 2022 · 0 comments · Fixed by #76334
Closed

opt: join reordering can produce incorrect query plans #76522

mgartner opened this issue Feb 14, 2022 · 0 comments · Fixed by #76334
Assignees
Labels
branch-release-20.2 Used to mark GA and release blockers, technical advisories, and bugs for 20.2 branch-release-21.1 Used to mark GA and release blockers, technical advisories, and bugs for 21.1 branch-release-21.2 Used to mark GA and release blockers, technical advisories, and bugs for 21.2 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. C-technical-advisory Caused a technical advisory S-0-visible-logical-error Database stores inconsistent data in some cases, or queries return invalid results silently. T-sql-queries SQL Queries Team

Comments

@mgartner
Copy link
Collaborator

mgartner commented Feb 14, 2022

Join reordering can produce query plans where filters in the original plan are missing, causing incorrect query results.

Below is a reproduction of the issue. Notice that the t2.should_not_be_eliminated = t5.should_not_be_eliminated filter is missing from the final query plan.

exec-ddl
CREATE TABLE t1 (
	a INT NOT NULL,
	b INT NOT NULL,
	PRIMARY KEY (a ASC, b ASC)
)
----

exec-ddl
CREATE TABLE t2 (
	a INT NOT NULL,
	c INT,
	should_not_be_eliminated INT,
	PRIMARY KEY (a ASC)
);
----

exec-ddl
CREATE TABLE t3 (
	a INT NOT NULL,
	d INT NOT NULL,
	PRIMARY KEY (a ASC, d ASC)
);
----

exec-ddl
CREATE TABLE t4 (
	e INT NOT NULL,
	f INT,
	g INT,
	PRIMARY KEY (e ASC)
);
----

exec-ddl
CREATE TABLE t5 (
	h INT NOT NULL,
	f INT NOT NULL,
	g INT NOT NULL,
	b INT,
	should_not_be_eliminated INT,
	c INT,
	PRIMARY KEY (h ASC, f ASC, g ASC)
);
----

# Give t1 many rows where a has many distincts.
exec-ddl
ALTER TABLE t1 INJECT STATISTICS '[
    {
        "columns": [
            "a"
        ],
        "created_at": "2022-01-17 12:51:38.433911",
        "distinct_count": 9161427,
        "null_count": 0,
        "row_count": 44484238
    }
]';
----

# Give t2 many rows where a has many distincts.
exec-ddl
ALTER TABLE t2 INJECT STATISTICS '[
    {
        "columns": [
            "a"
        ],
        "created_at": "2022-01-17 12:51:38.433911",
        "distinct_count": 17014025,
        "null_count": 0,
        "row_count": 17024553
    }
]';
----

# Give t3 many rows where a has many distincts.
exec-ddl
ALTER TABLE t3 INJECT STATISTICS '[
    {
        "columns": [
            "a"
        ],
        "created_at": "2022-01-17 12:51:38.433911",
        "distinct_count": 17187349,
        "null_count": 0,
        "row_count": 18138540
    }
]';
----

# Give t4 many rows where e has many distincts.
exec-ddl
ALTER TABLE t4 INJECT STATISTICS '[
    {
        "columns": [
            "e"
        ],
        "created_at": "2022-01-17 12:51:38.433911",
        "distinct_count": 346919,
        "null_count": 0,
        "row_count": 346109
    }
]';
----

# Give t5 few rows.
exec-ddl
ALTER TABLE t5 INJECT STATISTICS '[
    {
        "columns": [
            "h"
        ],
        "created_at": "2022-01-17 12:51:38.433911",
        "distinct_count": 119,
        "null_count": 0,
        "row_count": 119
    }
]';
----

# Notice that some of the filters are missing entirely:
#
#   t5.c = t2.c
#   t2.should_not_be_eliminated = t5.should_not_be_eliminated
#
opt
SELECT
  t2.a
FROM
  t1
  INNER JOIN t2 ON t1.a = t2.a
  INNER JOIN t3 ON t1.a = t3.a
  INNER JOIN t4 ON t3.d = t4.e
  INNER JOIN t5 ON
      t4.f = t5.f
      AND t4.g = t5.g
      AND t5.b = t1.b
      AND t5.c = t2.c
WHERE
  t1.a = 123456 AND t2.should_not_be_eliminated = t5.should_not_be_eliminated;
----
project
 ├── columns: a:5!null
 └── inner-join (lookup t1)
      ├── columns: t1.a:1!null t1.b:2!null t2.a:5!null t2.c:6!null t2.should_not_be_eliminated:7!null t3.a:10!null d:11!null e:14!null t4.f:15!null t4.g:16!null t5.f:20!null t5.g:21!null t5.b:22!null t5.should_not_be_eliminated:23!null t5.c:24!null
      ├── key columns: [5 22] = [1 2]
      ├── lookup columns are key
      ├── inner-join (lookup t2)
      │    ├── columns: t2.a:5!null t2.c:6 t2.should_not_be_eliminated:7 t3.a:10!null d:11!null e:14!null t4.f:15!null t4.g:16!null t5.f:20!null t5.g:21!null t5.b:22 t5.should_not_be_eliminated:23 t5.c:24
      │    ├── key columns: [10] = [5]
      │    ├── lookup columns are key
      │    ├── inner-join (hash)
      │    │    ├── columns: t3.a:10!null d:11!null e:14!null t4.f:15!null t4.g:16!null t5.f:20!null t5.g:21!null t5.b:22 t5.should_not_be_eliminated:23 t5.c:24
      │    │    ├── scan t5
      │    │    │    └── columns: t5.f:20!null t5.g:21!null t5.b:22 t5.should_not_be_eliminated:23 t5.c:24
      │    │    ├── inner-join (lookup t4)
      │    │    │    ├── columns: t3.a:10!null d:11!null e:14!null t4.f:15 t4.g:16
      │    │    │    ├── key columns: [11] = [14]
      │    │    │    ├── lookup columns are key
      │    │    │    ├── scan t3
      │    │    │    │    ├── columns: t3.a:10!null d:11!null
      │    │    │    │    └── constraint: /10/11: [/123456 - /123456]
      │    │    │    └── filters (true)
      │    │    └── filters
      │    │         ├── t4.f:15 = t5.f:20
      │    │         └── t4.g:16 = t5.g:21
      │    └── filters
      │         └── t2.a:5 = 123456
      └── filters
           └── t1.a:1 = 123456
@mgartner mgartner added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Feb 14, 2022
@mgartner mgartner self-assigned this Feb 14, 2022
@blathers-crl blathers-crl bot added the T-sql-queries SQL Queries Team label Feb 14, 2022
@rytaft rytaft added the S-0-visible-logical-error Database stores inconsistent data in some cases, or queries return invalid results silently. label Feb 14, 2022
mgartner added a commit to mgartner/cockroach that referenced this issue Feb 14, 2022
This commit eliminates logic in the `assoc`, `leftAsscom`, and
`rightAsscom` functions in the join order builder that aimed to prevent
generating "orphaned" predicates, where one or more referenced relations
are not in a join's input. In rare cases, this logic had the side effect
of creating invalid conflict rules for edges, which could prevent valid
predicates from being added to reordered join trees.

It is safe to remove these conditionals because they are unnecessary.
The CD-C algorithm already prevents generation of orphaned predicates by
checking that the total eligibility set (TES) is a subset of a join's
input vertices. In our implementation, this is handled by the
`checkNonInnerJoin` and `checkInnerJoin` functions.

Fixes cockroachdb#76522

Release note (bug fix): A bug has been fixed which caused the query optimizer
to omit join filters in rare cases when reordering joins, which could
result in incorrect query results. This bug was present since v20.2.
craig bot pushed a commit that referenced this issue Feb 15, 2022
74563: kv,kvcoord,sql: poison txnCoordSender after a retryable error r=lidorcarmel a=lidorcarmel

Previously kv users could lose parts of a transaction without getting an
error. After Send() returned a retryable error the state of txn got reset
which made it usable again. If the caller ignored the error they could
continue applying more operations without realizing the first part of the
transaction was discarded. See more details in the issue (#22615).

The simple case example is where the retryable closure of DB.Txn() returns
nil instead of returning the retryable error back to the retry loop - in this
case the retry loop declares success without realizing we lost the first part
of the transaction (all the operations before the retryable error).

This PR leaves the txn in a "poisoned" state after encountering an error, so
that all future operations fail fast. The caller is therefore expected to
reset the txn handle back to a usable state intentionally, by calling
Txn.PrepareForRetry(). In the simple case of DB.Txn() the retry loop will
reset the handle and run the retry even if the callback returned nil.

Closes #22615

Release note: None

74662: tsdb: expand mem per worker based on sql pool size r=dhartunian a=dhartunian

Previously, the memory limit for all `tsdb` workers was set at a static
64MiB. This cap created issues seen in #24018 where this limit was hit
on a 30 node cluster. To alleviate the issue, the number of workers was
reduced, raising the per-worker allocation.

We've currently hit this limit again as part of load testing with larger
clusters and have decided to make the per-query worker memory limit
dynamic. The per-worker limit is now raised based on the amount of memory
available to the SQL Pool via the `MemoryPoolSize` configuration
variable. This is set to be 25% of the system memory by default. The
`tsdb` memory cap per-worker is now doubled until it reaches `1/128` of
the memory pool setting.

For example, on a node with 128 - 256 GiB of memory, this will
correspond to 512 MiB allocated for all running `tsdb` queries.

In addition, the ts server is now connected to the same `BytesMonitor`
instance as the SQL memory monitor and workers will becapped at double
the query limit. Results are monitored as before but a cap is not
introduced there since we didn't have one present previously.

This behavior is gated behind a private cluster setting that's enabled
by default and sets the ratio at 1/128 of the SQL memory pool.

Resolves #72986

Release note (ops change): customers running clusters with 240 nodes or
more can effectively access tsdb metrics.

75677: randgen: add PopulateRandTable r=mgartner a=msbutler

PopulateRandTable populates the caller's table with random data. This helper
function aims to make it easier for engineers to develop randomized tests that
leverage randgen / sqlsmith.

Informs #72345

Release note: None

76334: opt: fix missing filters after join reordering r=mgartner a=mgartner

#### opt: add TES, SES, and rules to reorderjoins

This commit updates the output of the `reorderjoins` opt test command to
display the initial state of the `JoinOrderBuilder`. It adds additional
information to the output including the TES, SES, and conflict rules for
each edge.

Release note: None

#### opt: fix missing filters after join reordering

This commit eliminates logic in the `assoc`, `leftAsscom`, and
`rightAsscom` functions in the join order builder that aimed to prevent
generating "orphaned" predicates, where one or more referenced relations
are not in a join's input. In rare cases, this logic had the side effect
of creating invalid conflict rules for edges, which could prevent valid
predicates from being added to reordered join trees.

It is safe to remove these conditionals because they are unnecessary.
The CD-C algorithm already prevents generation of orphaned predicates by
checking that the total eligibility set (TES) is a subset of a join's
input vertices. In our implementation, this is handled by the
`checkNonInnerJoin` and `checkInnerJoin` functions.

Fixes #76522

Release note (bug fix): A bug has been fixed which caused the query optimizer
to omit join filters in rare cases when reordering joins, which could
result in incorrect query results. This bug was present since v20.2.


Co-authored-by: Lidor Carmel <[email protected]>
Co-authored-by: David Hartunian <[email protected]>
Co-authored-by: Michael Butler <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>
@craig craig bot closed this as completed in 66d8865 Feb 15, 2022
mgartner added a commit to mgartner/cockroach that referenced this issue Feb 15, 2022
This commit eliminates logic in the `assoc`, `leftAsscom`, and
`rightAsscom` functions in the join order builder that aimed to prevent
generating "orphaned" predicates, where one or more referenced relations
are not in a join's input. In rare cases, this logic had the side effect
of creating invalid conflict rules for edges, which could prevent valid
predicates from being added to reordered join trees.

It is safe to remove these conditionals because they are unnecessary.
The CD-C algorithm already prevents generation of orphaned predicates by
checking that the total eligibility set (TES) is a subset of a join's
input vertices. In our implementation, this is handled by the
`checkNonInnerJoin` and `checkInnerJoin` functions.

Fixes cockroachdb#76522

Release note (bug fix): A bug has been fixed which caused the query optimizer
to omit join filters in rare cases when reordering joins, which could
result in incorrect query results. This bug was present since v20.2.
mgartner added a commit to mgartner/cockroach that referenced this issue Feb 15, 2022
This commit eliminates logic in the `assoc`, `leftAsscom`, and
`rightAsscom` functions in the join order builder that aimed to prevent
generating "orphaned" predicates, where one or more referenced relations
are not in a join's input. In rare cases, this logic had the side effect
of creating invalid conflict rules for edges, which could prevent valid
predicates from being added to reordered join trees.

It is safe to remove these conditionals because they are unnecessary.
The CD-C algorithm already prevents generation of orphaned predicates by
checking that the total eligibility set (TES) is a subset of a join's
input vertices. In our implementation, this is handled by the
`checkNonInnerJoin` and `checkInnerJoin` functions.

Fixes cockroachdb#76522

Release note (bug fix): A bug has been fixed which caused the query optimizer
to omit join filters in rare cases when reordering joins, which could
result in incorrect query results. This bug was present since v20.2.
RajivTS pushed a commit to RajivTS/cockroach that referenced this issue Mar 6, 2022
This commit eliminates logic in the `assoc`, `leftAsscom`, and
`rightAsscom` functions in the join order builder that aimed to prevent
generating "orphaned" predicates, where one or more referenced relations
are not in a join's input. In rare cases, this logic had the side effect
of creating invalid conflict rules for edges, which could prevent valid
predicates from being added to reordered join trees.

It is safe to remove these conditionals because they are unnecessary.
The CD-C algorithm already prevents generation of orphaned predicates by
checking that the total eligibility set (TES) is a subset of a join's
input vertices. In our implementation, this is handled by the
`checkNonInnerJoin` and `checkInnerJoin` functions.

Fixes cockroachdb#76522

Release note (bug fix): A bug has been fixed which caused the query optimizer
to omit join filters in rare cases when reordering joins, which could
result in incorrect query results. This bug was present since v20.2.
@mgartner mgartner moved this to Done in SQL Queries Jul 24, 2023
@rytaft rytaft added C-technical-advisory Caused a technical advisory branch-release-21.2 Used to mark GA and release blockers, technical advisories, and bugs for 21.2 branch-release-21.1 Used to mark GA and release blockers, technical advisories, and bugs for 21.1 branch-release-20.2 Used to mark GA and release blockers, technical advisories, and bugs for 20.2 labels Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch-release-20.2 Used to mark GA and release blockers, technical advisories, and bugs for 20.2 branch-release-21.1 Used to mark GA and release blockers, technical advisories, and bugs for 21.1 branch-release-21.2 Used to mark GA and release blockers, technical advisories, and bugs for 21.2 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. C-technical-advisory Caused a technical advisory S-0-visible-logical-error Database stores inconsistent data in some cases, or queries return invalid results silently. T-sql-queries SQL Queries Team
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants