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.

Sign up for GitHub

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

Jump to bottom

Allow AS OF SYSTEM TIME (AOST) to be passed to EXPLAIN #43294

Closed
rmloveland opened this issue Dec 18, 2019 · 1 comment · Fixed by #43296
Closed

Allow AS OF SYSTEM TIME (AOST) to be passed to EXPLAIN #43294

rmloveland opened this issue Dec 18, 2019 · 1 comment · Fixed by #43296
Assignees
@RaduBerinde
Labels
A-sql-optimizer SQL logical planning and optimizations. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)

Comments

@rmloveland
Copy link
Collaborator

rmloveland commented Dec 18, 2019 •
edited
Loading

In version 19.2.1, if you stick EXPLAIN on the front of a query that includes AS OF SYSTEM TIME, it fails with the following error message:

pq: AS OF SYSTEM TIME must be provided on a top-level statement

I would like to be able to take a query that includes AS OF SYSTEM TIME and pass it to EXPLAIN without having to edit out the AOST portion.

More generally, this was surprising to me because my expectation of EXPLAIN is that I can pretty much just "slap an EXPLAIN on it" with any arbitrary SQL query.

In the SQL shell, this is the difference between:

  1. up arrow
  2. ctrl-a (beginning of line)
  3. type "EXPLAIN "

vs. doing a bunch of possibly painful inline edits, including maybe making a mistake so that the query that gets EXPLAINed does not exactly match your intended query, etc.

From client code, it could also be unpleasant depending on your environment, since you couldn't just prepend an EXPLAIN e.g.

$query = 'EXPLAIN ' . $query;

In this case, I was trying to see if / how the EXPLAIN output would actually differ if AOST were used on a query. In the AOST docs we say re: performance that

This clause can be used to read historical data (also known as "time travel queries") and can also be advantageous for performance as it decreases transaction conflicts

but it's pretty vague. I wanted to run an EXPLAIN to see if there were other performance implications to the query plan that could be seen vs. running the same query without AOST.

It also makes documenting things more verbose in some cases (such as work I'm doing on this docs PR since the workflow is not (1) run query, (2) slap an EXPLAIN on the query, it's the whole inline-edit-and-don't-make-a-mistake thing mentioned above, which also requires mentioning this limitation inline in those (unrelated) docs, which adds cognitive weight to the user.

From a UX perspective, this limitation is a bit unfortunate if we are expecting people to write AOST queries on a regular basis to get good performance, which I think we are.

Related issues: I searched around and did find #30534 which may be the technical reason for this limitation but is not about this particular use case.

Exact version info:

SELECT VERSION();
                                         version                                         
+---------------------------------------------------------------------------------------+
  CockroachDB CCL v19.2.1 (x86_64-apple-darwin14, built 2019/11/18 23:17:47, go1.12.12)  
(1 row)
@awoods187 awoods187 added A-sql-optimizer SQL logical planning and optimizations. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) labels Dec 18, 2019
@awoods187
Copy link
Contributor

@RaduBerinde this seems like something we'd want to do and backport

@rmloveland rmloveland mentioned this issue Dec 18, 2019
Closed
@RaduBerinde RaduBerinde self-assigned this Dec 18, 2019
@RaduBerinde RaduBerinde mentioned this issue Dec 18, 2019
Merged
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Dec 18, 2019
@RaduBerinde
sql: support EXPLAIN with AS OF SYSTEM TIME
6219bc7 
We apparently can't stick an `EXPLAIN` in front of a query that uses
AOST. The fix is very easy, we need an extra case for the logic that
figures out the statement-wide timestamp.

Note that if we want to do `SELECT FROM [EXPLAIN ...]`, in that case
we still need to add AS OF SYSTEM TIME to the outer clause as usual.

Fixes cockroachdb#43294.

Release note (bug fix): EXPLAIN can now be used with statements that
use AS OF SYSTEM TIME.
@RaduBerinde RaduBerinde mentioned this issue Dec 18, 2019
Merged
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Dec 18, 2019
@RaduBerinde
sql: support EXPLAIN with AS OF SYSTEM TIME
88320a2 
We apparently can't stick an `EXPLAIN` in front of a query that uses
AOST. The fix is very easy, we need an extra case for the logic that
figures out the statement-wide timestamp.

Note that if we want to do `SELECT FROM [EXPLAIN ...]`, in that case
we still need to add AS OF SYSTEM TIME to the outer clause as usual.

Fixes cockroachdb#43294.

Release note (bug fix): EXPLAIN can now be used with statements that
use AS OF SYSTEM TIME.
This was referenced Dec 18, 2019
Merged
Merged
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Dec 18, 2019
@RaduBerinde
sql: support EXPLAIN with AS OF SYSTEM TIME
c3c9665 
We apparently can't stick an `EXPLAIN` in front of a query that uses
AOST. The fix is very easy, we need an extra case for the logic that
figures out the statement-wide timestamp.

Note that if we want to do `SELECT FROM [EXPLAIN ...]`, in that case
we still need to add AS OF SYSTEM TIME to the outer clause as usual.

Fixes cockroachdb#43294.

Release note (bug fix): EXPLAIN can now be used with statements that
use AS OF SYSTEM TIME.
craig bot pushed a commit that referenced this issue Dec 18, 2019
@knz @RaduBerinde
Merge #43032 #43296 #43300 #43302
0e59890 
43032: kv: remove `GetMeta`, `AugmentMeta` and `TxnCoordMeta` r=knz a=knz

Recommended/requested by @nvanbenschoten. 
Discussed with @andreimatei. 
Prerequisite to completing #42854.

Prior to this patch, the same data structure `TxnCoordMeta` was used
both to initialize a LeafTxn from a RootTxn, and a RootTxn from a
LeafTxn. Moreover, the same method on `TxnCoordSender` (`AugmentMeta`)
was used to "configure" a txn into a root or a leaf, and to update
a root from the final state of leaves.

This was causing difficult questions when adding features (all the
fields in TxnCoordMeta needed to produce effects in one direction
and no-ops in the other). It was also making it hard to read and
understand the API.

This patch alleviates this problem by separating the two protocols:

```go
// From roots:
func (txn *Txn) GetLeafTxnInputStateOrRejectClient(context.Context) (roachpb.LeafTxnInputState, error)

// to create a new leaf:
func NewLeafTxn(context.Context, *DB, roachpb.NodeID, *roachpb.LeafTxnInputState) *Txn

// From leaves, at end of use:
func (txn *Txn) GetLeafTxnFinalState(context.Context) roachpb.LeafTxnFinalState

// Back into roots:
func (txn *Txn) UpdateRootWithLeafFinalState(context.Context, tfs *roachpb.LeafTxnFinalState)
```

Additionally, this patch:

- removes the general-purpose `Serialize()` method, and replaces it
  by `TestingCloneTxn()` specifically purposed for use in testing.

- removes direct access to the TxnMeta `WriteTimestamp` in the SQL
  conn executor (to establish a high water mark for table lease
  expiration), and replaces it by a call to a new method
  `ProvisionalCommitTimestamp()`.

Release note: None

43296: sql: support EXPLAIN with AS OF SYSTEM TIME r=RaduBerinde a=RaduBerinde

We apparently can't stick an `EXPLAIN` in front of a query that uses
AOST. The fix is very easy, we need an extra case for the logic that
figures out the statement-wide timestamp.

Note that if we want to do `SELECT FROM [EXPLAIN ...]`, in that case
we still need to add AS OF SYSTEM TIME to the outer clause as usual.

Fixes #43294.

Release note (bug fix): EXPLAIN can now be used with statements that
use AS OF SYSTEM TIME.

43300: blobs: Stat method bug fix r=g3orgia a=g3orgia

The stat method has a typo/bug, it return nil
instead of err. This PR fixes it.

Release note: None

43302: scripts: fix the release note script to pick up more backports r=knz a=knz

Prior to this patch, the release note script was only recognizing PR
merges from either of two formats:

- from Bors, starting with `Merge #xxx #yyy`
- from Github, starting with `Merge pull request #xxx from ...`

Sometime in 2019, Github has started populating merge commits using
a different format, using instead the title of the PR followed by the
PR number between parentheses.

This new format was not recognized by the script and caused it to skip
over these merges (and all their underlying commits) and list them in
the section "changes without a release note annotation".

This patch fixes it.

Release note: None

Co-authored-by: Raphael 'kena' Poss <[email protected]>
Co-authored-by: Radu Berinde <[email protected]>
Co-authored-by: Georgia Hong <[email protected]>
@craig craig bot closed this as completed in fd94dad Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@RaduBerinde RaduBerinde

Labels
A-sql-optimizer SQL logical planning and optimizations. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

sql: support EXPLAIN with AS OF SYSTEM TIME RaduBerinde/cockroach
3 participants
@rmloveland @RaduBerinde @awoods187