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: now() doesn't get evaluated before index selection #18836

Open
jordanlewis opened this issue Sep 27, 2017 · 10 comments

Comments

6 participants
@jordanlewis
Copy link
Member

commented Sep 27, 2017

root@:26257/test> CREATE TABLE bydate(a TIMESTAMP NOT NULL);
CREATE TABLE
root@:26257/test> explain SELECT * FROM bydate WHERE a > (now() - '1h':::interval);
+-------+--------+-------+----------------+
| Level |  Type  | Field |  Description   |
+-------+--------+-------+----------------+
|     0 | render |       |                |
|     1 | scan   |       |                |
|     1 |        | table | bydate@primary |
|     1 |        | spans | ALL            |
+-------+--------+-------+----------------+
(4 rows)

Even though we've added a constraint to the index key, the query planner produces a full table scan. This is incorrect. We should be able to evaluate now() - '1h':::interval before index selection so we can add it as a constraint to the index span.

@petermattis

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2017

now() is marked as impure while the comment about impure says:

	// Set to true when a function potentially returns a different value
	// when called in the same statement with the same parameters.
	// e.g.: random(), clock_timestamp(). Some functions like now()
	// return the same value in the same statement, but different values
	// in separate statements, and should not be marked as impure.

If we marked now() were marked as pure I believe it would participate in constant folding.

@knz

This comment has been minimized.

Copy link
Member

commented Sep 27, 2017

It's not pure because the txn timestamp is reset across retries. We want now() to have a timestamp aligned with the timestamp of the txn as finally committed, not the timestamp of the first retry attempt.

@knz

This comment has been minimized.

Copy link
Member

commented Sep 27, 2017

(I say "we want" in that the Jepsen tests have shown us this to be necessary for cases when people use now() to reflect progress of their changes. We could have two txn timestamp built-in functions side by side, and document the difference.)

@bdarnell

This comment has been minimized.

Copy link
Member

commented Sep 29, 2017

Alternately, when an impure function is used in a where clause like this, we could evaluate it before planning and then re-plan on any transaction retry.

@bdarnell

This comment has been minimized.

Copy link
Member

commented Sep 29, 2017

And as a workaround for applications affected by this, the current time can be computed on the client side and passed in as a query parameter: SELECT * FROM bydate WHERE a > ($1 - '1h':::interval)

@knz

This comment has been minimized.

Copy link
Member

commented Sep 29, 2017

re-planning on each attempt if time-based functions are used is ... interesting. With the "sql leaf data" proposal, we can avoid doing all the planning and merely defer the (re)computation of spans to each retry attempt.

@knz

This comment has been minimized.

Copy link
Member

commented Jan 22, 2018

Unfortunately this needs more research and thus has to slip beyond 2.0. Bumping to 2.1. It would be nice to integrate this in the perf & optimizations projects for the next release somehow. cc @jordanlewis and @petermattis for technical strategizing.

@knz

This comment has been minimized.

Copy link
Member

commented Jun 11, 2018

I found the proper direction for a solution, see #26582.

@andy-kimball andy-kimball modified the milestones: 2.1, 2.2 Aug 24, 2018

@andy-kimball

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2018

Pushing this to 2.2, since we don't yet have the machinery to do something better.

@andy-kimball andy-kimball added this to To do in SQL Planning via automation Aug 25, 2018

@andy-kimball andy-kimball moved this from To do to Higher Priority Backlog in SQL Planning Aug 25, 2018

@petermattis petermattis removed this from the 2.2 milestone Oct 5, 2018

@bobvawter bobvawter self-assigned this Oct 8, 2018

@knz

This comment has been minimized.

Copy link
Member

commented Jan 13, 2019

User faced this see #33696

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.