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

sql: incorrect number of rows when we have an offset over a limit #38659

Closed
ridwanmsharif opened this issue Jul 3, 2019 · 1 comment · Fixed by #38667
Closed

sql: incorrect number of rows when we have an offset over a limit #38659

ridwanmsharif opened this issue Jul 3, 2019 · 1 comment · Fixed by #38667
Assignees
Labels
A-sql-execution Relating to SQL execution. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@ridwanmsharif
Copy link

ridwanmsharif commented Jul 3, 2019

Out limit doesn't seem to be right when we have an offset above it.
Consider the following table:

  col
+----+
   1
   2
   3
   4
   5
   6
   7
   8
   9
  10
(10 rows)

And the query:

SELECT * FROM (SELECT * FROM tab ORDER BY col LIMIT 5) ORDER BY a OFFSET 3

We expect to see (which is what postgres returns)

  4
  5
(2 rows)

But instead we get:

  4
  5
  6
  7
  8
(5 rows)
@ridwanmsharif ridwanmsharif added the A-sql-execution Relating to SQL execution. label Jul 3, 2019
@jordanlewis jordanlewis added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Jul 3, 2019
@jordanlewis
Copy link
Member

jordanlewis commented Jul 3, 2019

Thanks. This looks like a bug in distsql physical planning.

Explain plans for your scenario:

root@127.0.0.1:51341/defaultdb> explain SELECT * FROM (SELECT * FROM tab ORDER BY a LIMIT 5) ORDER BY a OFFSET 3;
    tree    | field  | description
+-----------+--------+-------------+
  limit     |        |
   │        | offset | 3
   └── scan |        |
            | table  | tab@primary
            | spans  | ALL
            | limit  | 5
(6 rows)

Time: 541µs

root@127.0.0.1:51341/defaultdb> explain(distsql) SELECT * FROM (SELECT * FROM tab ORDER BY a LIMIT 5) ORDER BY a OFFSET 3;
  automatic |                                                                                                                                                                url
+-----------+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
    true    | https://cockroachdb.github.io/distsqlplan/decode.html#eJyMjzFLBDEUhHt_xTL1E3cRm1S2B-LJaScpcpt3R2A3CXlvQTny3-WSQiyEK-ebZGbeBTF5fnUrC8wnJlhCLmlmkVSuqD_Y-S-YkRBi3vSKLWFOhWEu0KALw-DDHRc-sPNcHkYQPKsLS4vNJayufD-rO4Lwnl0UM9yD8BLWoMPTsD-dhHV4hK2EtOlvi6g7M8xU6fYlB5acovCfEf8lj9US2J-5XytpKzO_lTS3mi737V8DnkW7O3Wxi92qtt79BAAA___H2G4Y
(1 row)

Time: 9.758ms

Note that the physical plan combines the offset and limit in a bogus way.

craig bot pushed a commit that referenced this issue Jul 8, 2019
38667: distsqlplan: bugfix to planning offsets on top of limits r=jordanlewis a=jordanlewis

Fixes #38659.

DistSQL physical planning removes limitNodes when it can, by pushing the
embedded limit and offset into the most recent processor's
PostProcessSpec when possible.

This was previously buggy, when trying to plan an offset on top of a
processor that already had a limit inside, because the code simply set
the offset on the processor without regard for the fact that offsets are
always applied first at runtime, before limits.

Release note (bug fix): correct issue that caused certain plans that
contained both offsets and limits to return more rows than desired.

Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com>
@craig craig bot closed this as completed in #38667 Jul 8, 2019
craig bot pushed a commit that referenced this issue Jul 9, 2019
38570: opt: fix panic recovery for error handling r=RaduBerinde a=RaduBerinde

The major entry points in the optimizer catch all panics that throw an
error and converts them to errors. Unfortunately, this also catches
runtime errors (in which case we convert them to errors and lose the
stack trace).

This change adds a `ShouldCatch` helper which determines if we should
return a thrown object as an error. If the object is a
`runtime.Error`, it gets wrapped by an AssertionFailed error which
will cause correct error handling (stack trace, sentry reporting, etc).

As part of this change, we are also removing wrappers like
`builderError`, which are no longer useful. We fix the opt tester to
fail with the full error information (using `%+v`) for assertion
errors.

Release note: None

38660: opt: push limit into offset r=ridwanmsharif a=ridwanmsharif

This change pushes the limit into an offset whenever possible.
This shouldn't worsen any plan but does allow the `GetLimitedScans`
rule to fire in more scenarios.

Fixes #30416.
~~This is currently blocked on #38659.~~

Release note: None

38743: roachtest: skip jepsen/multi-register r=god a=nvanbenschoten

There's no use running this every night until #36431 is fixed.

Release note: None

38746: roachtest: don't reuse clusters after test failure r=andreimatei a=andreimatei

We've had a case where a cluster got messed up somehow and then a bunch
of tests that tried to reuse it failed. This patch employes a big hammer
and makes it so that we don't reuse a cluster after test failure (which
failure can be cluster related or not).

Release note: None

38766: scripts/release-notes.py: help the user with --from/--until r=lhirata a=knz

Requested by @lhirata

Release note: None

Co-authored-by: Radu Berinde <radu@cockroachlabs.com>
Co-authored-by: Ridwan Sharif <ridwan@cockroachlabs.com>
Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: Andrei Matei <andrei@cockroachlabs.com>
Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-execution Relating to SQL execution. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants