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

cherry-pick 1.1: sql: ensure that AS OF SYSTEM TIME is handled consistently #20286

Merged
merged 1 commit into from Nov 27, 2017

Conversation

knz
Copy link
Contributor

@knz knz commented Nov 27, 2017

Cherry-pick of #20267.

Prior to this patch, two inconsistencies were present in the code:

  • a user-visible inconsistency: AS OF SYSTEM TIME (henceforth
    referred to as "AOST") was accepted anywhere in a query as soon as
    it was present at the top level SELECT statement, including with
    conflicting timestamps. Accepting AOST in multiple places doesn't
    feel undesirable, but allowing conflicting timestamps to be
    specified is unsound with the current underlying mechanism (a shared
    timestamp for the entire transaction).

  • an internal inconsistency, visible to CockroachDB developers: the
    presence of an AOST clause during planning was erroneously conflated
    with the flag that disables caching of table descriptors
    (planner.avoidCachedDescriptors). This is erroneous because,
    although AOST implies avoidCachedDescriptors == true (we can't
    use the cache while time travelling), the converse is not true: when
    processing view descriptors (and perhaps, in the future, for other
    reasons), we also disable descriptor caching although AOST is not
    involved.

This patch rectifies this situation as follows:

  • a new planner flag asOfSystemTime is introduced to indicate
    that an AOST clause was properly recognized at the top level.

  • the logic that allows or refuses AOST clauses in FROM clauses in
    arbitrarily nested SELECT clauses (including, potentially, those
    expanded from views), is modified to use this new flag.

  • the timestamps of AOST clauses, if multiple are specified, are
    checked to be equal to the one set at the top level. This
    restriction might be lifted in the future if we ever support
    different AOST clauses per data source.

In addition, the error message when an AOST clause is not given in the
proper syntactic position is improved to hint where it should be
placed instead.


Release note (sql, bug fix): it is not possible any more to indicate
conflicting AS OF SYSTEM TIME clauses in different part of a query.

cc @cockroachdb/release

@knz knz requested review from maddyblue and a team November 27, 2017 20:47
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Prior to this patch, two inconsistencies were present in the code:

- a user-visible inconsistency: `AS OF SYSTEM TIME` (henceforth
  referred to as "AOST") was accepted anywhere in a query as soon as
  it was present at the top level SELECT statement, including with
  conflicting timestamps. Accepting AOST in multiple places doesn't
  feel undesirable, but allowing conflicting timestamps to be
  specified is unsound with the current underlying mechanism (a shared
  timestamp for the entire transaction).

- an internal inconsistency, visible to CockroachDB developers: the
  presence of an AOST clause during planning was erroneously conflated
  with the flag that disables caching of table descriptors
  (`planner.avoidCachedDescriptors`). This is erroneous because,
  although AOST *implies* `avoidCachedDescriptors == true` (we can't
  use the cache while time travelling), the converse is not true: when
  processing view descriptors (and perhaps, in the future, for other
  reasons), we also disable descriptor caching although AOST is not
  involved.

This patch rectifies this situation as follows:

- a new planner flag `asOfSystemTime` is introduced to indicate
  that an AOST clause was properly recognized at the top level.

- the logic that allows or refuses AOST clauses in FROM clauses in
  arbitrarily nested SELECT clauses (including, potentially, those
  expanded from views), is modified to use this new flag.

- the timestamps of AOST clauses, if multiple are specified, are
  checked to be equal to the one set at the top level. This
  restriction might be lifted in the future if we ever support
  different AOST clauses per data source.

In addition, the error message when an AOST clause is not given in the
proper syntactic position is improved to hint where it should be
placed instead.

----

Release note (sql, bug fix): it is not possible any more to indicate
conflicting `AS OF SYSTEM TIME` clauses in different part of a query.
@knz knz merged commit de82822 into cockroachdb:release-1.1 Nov 27, 2017
@knz knz deleted the 20171127-cherrypick-20267 branch November 27, 2017 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants