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

release-21.2: opt,tree: fix bugs with Next(), Prev(), and histogram calculation for DTimeTZ #75172

Merged
merged 2 commits into from
Jan 20, 2022

Conversation

rytaft
Copy link
Collaborator

@rytaft rytaft commented Jan 19, 2022

Backport 2/2 commits from #74914.

/cc @cockroachdb/release


sql/sem/tree: fix Next() and Prev() for DTimeTZ

Prior to this commit, the DTimeTZ functions Next() and Prev()
could skip over valid values according to the ordering of DTimeTZ
values in an index (which matches the ordering defined by the
TimeTZ functions After() and Before()).

This commit fixes these functions so that Next() now returns the smallest
valid DTimeTZ that is greater than the receiver, and Prev() returns
the largest valid DTimeTZ that is less than the receiver. This is
an important invariant that the optimizer relies on when building index
constraints.

Fixes #74912

Release note (bug fix): Fixed a bug that could occur when a TIMETZ
column was indexed, and a query predicate constrained that column using
a < or > operator with a timetz constant. If the column contained values
with time zones that did not match the time zone of the timetz constant,
it was possible that not all matching values could be returned by the
query. Specifically, the results may not have included values within one
microsecond of the predicate's absolute time. This bug was introduced
when the timetz datatype was first added in 20.1. It exists on all
versions of 20.1, 20.2, 21.1, and 21.2 prior to this patch.

opt: fix bug in histogram calculation for TimeTZ

This commit fixes a bug in the histogram estimation code for TimeTZ
that made the faulty assumption that TimeTZ values are ordered by
TimeOfDay. This is incorrect since it does not take the OffsetSecs
into account. As a result, it was possible to estimate that the size
of a histogram bucket was negative, which caused problems in the
statistics estimation code. This commit fixes the problem by taking
into account both TimeOfDay and OffsetSecs when estimating the size of
a bucket in a TimeTZ histogram.

Fixes #74667

Release note (bug fix): Fixed an internal error, "estimated row count must
be non-zero", that could occur during planning for queries over a table
with a TimeTZ column. This error was due to a faulty assumption in the
statistics estimation code about ordering of TimeTZ values, which has now
been fixed. The error could occur when TimeTZ values used in the query had
a different time zone offset than the TimeTZ values stored in the table.

Prior to this commit, the DTimeTZ functions Next() and Prev()
could skip over valid values according to the ordering of DTimeTZ
values in an index (which matches the ordering defined by the
TimeTZ functions After() and Before()).

This commit fixes these functions so that Next() now returns smallest
valid DTimeTZ that is greater than the receiver, and Prev() returns
the largest valid DTimeTZ that is less than the receiver. This is
an important invariant that the optimizer relies on when building index
constraints.

Fixes cockroachdb#74912

Release note (bug fix): Fixed a bug that could occur when a TIMETZ
column was indexed, and a query predicate constrained that column using
a < or > operator with a timetz constant. If the column contained values
with time zones that did not match the time zone of the timetz constant,
it was possible that not all matching values could be returned by the
query. Specifically, the results may not have included values within one
microsecond of the predicate's absolute time. This bug was introduced
when the timetz datatype was first added in 20.1. It exists on all
versions of 20.1, 20.2, 21.1, and 21.2 prior to this patch.
@rytaft rytaft requested review from mgartner, otan and a team January 19, 2022 20:55
@rytaft rytaft requested a review from a team as a code owner January 19, 2022 20:55
@blathers-crl
Copy link

blathers-crl bot commented Jan 19, 2022

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Patches should only be created for serious issues or test-only changes.
  • Patches should not break backwards-compatibility.
  • Patches should change as little code as possible.
  • Patches should not change on-disk formats or node communication protocols.
  • Patches should not add new functionality.
  • Patches must not add, edit, or otherwise modify cluster versions; or add version gates.
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

  • What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
  • Will this work in a cluster of mixed patch versions? Did we test that?
  • If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

@cockroach-teamcity
Copy link
Member

This change is Reviewable

This commit fixes a bug in the histogram estimation code for TimeTZ
that made the faulty assumption that TimeTZ values are ordered by
TimeOfDay. This is incorrect since it does not take the OffsetSecs
into account. As a result, it was possible to estimate that the size
of a histogram bucket was negative, which caused problems in the
statistics estimation code. This commit fixes the problem by taking
into account both TimeOfDay and OffsetSecs when estimating the size of
a bucket in a TimeTZ histogram.

Fixes cockroachdb#74667

Release note (bug fix): Fixed an internal error, "estimated row count must
be non-zero", that could occur during planning for queries over a table
with a TimeTZ column. This error was due to a faulty assumption in the
statistics estimation code about ordering of TimeTZ values, which has now
been fixed. The error could occur when TimeTZ values used in the query had
a different time zone offset than the TimeTZ values stored in the table.
@rytaft rytaft merged commit 252bba4 into cockroachdb:release-21.2 Jan 20, 2022
@rytaft rytaft deleted the backport21.2-74914 branch January 20, 2022 16:57
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.

3 participants