storage: some benchmarks fail in CI with crash in newPebbleIterator
#97061
Assignees
Labels
C-bug
Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
C-test-failure
Broken test (automatically or manually discovered).
Comments
RaduBerinde
added
C-bug
RaduBerinde
changed the title
newPebbleIteratornewPebbleIterator
RaduBerinde
changed the title
newPebbleIteratornewPebbleIterator
|
Actually, it looks like we |
|
The failures have a much simpler cause - the benchmarks set format versions that are too low. |
|
Came here to report this crash1 in error during engine close: leaked iterators: currentI looked at the test and it's not obviously leaking an iterator. Footnotes |
|
The real panic is Maybe you can review #97066 and bors it if it looks good. |
|
By the way, I was considering calling |
craig bot
pushed a commit
that referenced
this issue
Feb 14, 2023
97058: cloud: bump version to v22.2.4 r=ZhouXing19 a=ZhouXing19 Update the Docker image tag version in our Kubernetes config files to v22.2.4. Epic: None Release note: None 97060: roachtest: upgrade the `predecessor_version.json` for 22.2.4 r=ZhouXing19 a=ZhouXing19 Epic: None Release note: None 97066: storage: never set a too-old format version r=RaduBerinde a=RaduBerinde This change asserts that we never set a pebble FormatVersion that is older than the minimum supported by our code. Benchmarks that set a lower value are updated (and BenchmarkTimeBoundIterate is reenabled). These benchmarks can fail when the code exercises a feature like range keys that is not supported by the older format. Fixes #97061 Release note: None Epic: none Co-authored-by: Jane Xing <[email protected]> Co-authored-by: Radu Berinde <[email protected]>
|
I still see a similar failure: https://teamcity.cockroachdb.com/viewLog.html?buildId=8704916&buildTypeId=Cockroach_Ci_Tests_Bench&tab=artifacts#%2Ftmp%2F_tmp%2Fe1ab32a24518a46d10d3b8d78552a36a;%2Ftmp%2F_tmp%2F99262b2ae388e806cec3034f997f74ca Cannot reproduce locally. I'm back to the initial hypothesis that there are somehow old fixtures accessible in CI. |
RaduBerinde
added a commit
to RaduBerinde/cockroach
that referenced
this issue
Feb 14, 2023
This benchmark is sometimes failing in CI and I cannot reproduce locally. The error is consistent with a leftover fixture from an older version (which shouldn't happen because CI should be cleaning up the repo). This change adds the version to the fixture name so that this kind of cross-version problem cannot occur. This is a good idea regardless of the CI issue. The logging around the fixture is also improved. Informs cockroachdb#97061 Release note: None Epic: none
|
After talking to @rickystewart, it is possible that the directory would be reused on the bazel CI, because the bazel cache (and sandbox) is not cleared between builds. |
RaduBerinde
added a commit
to RaduBerinde/cockroach
that referenced
this issue
Feb 14, 2023
Some storage-related benchmarks leave fixtures around in `.gitignore`d directories. There is potential for fixtures from old versions to cause failures. We are seeing some CI failures that would be explained by this (we're not sure yet if it's possible). In any case, it is better to include the version in the fixture name to avid this (which can be a problem even locally). The logging around the fixture location is also improved. Informs cockroachdb#97061 Release note: None Epic: none
craig bot
pushed a commit
that referenced
this issue
Feb 15, 2023
96295: changefeedccl: Rename and remove CDC functions r=miretskiy a=miretskiy
This PR renames and removes CDC specific functions, while maintaining backward compatability.
* `cdc_is_delete()` function removed. It is replaced
with `event_op()` function which returns a string describing
the type of event. If the changefeed is running with `diff`
option, then this function returns `insert`, `update`, or
`delete`. If changefeed is running without the `diff` option,
we can't tell an update from insert, so this function returns
`upsert` or `delete`.
* `cdc_mvcc_timestamp()` function removed. This information can be accessed
via cockroach standard system column `crdb_internal_mvcc_timestamp`.
The same timestamp column is avaiable in the previous row state
`(cdc_prev).crdb_internal_mvcc_timestamp`
* `cdc_updated_timestamp()` function renamed as `event_schema_timestamp()`
Fixes #92482
Epic: CRDB-17161
Release note (enterprise change): Deprecate and replace some of the changefeed specific functions made available in preview mode in 22.2
release. While these functions are deprecated, old changefeed
transformations should continue to function properly.
Customers are encouraged to closely monitor their changefeeds
created during upgrade. While effort was made to maintain backward
compatibility, the updated changefeed transformation may produce
slightly different output (different column names, etc).
97041: parser: add `QUERY` to `bare_label_keyword` r=ZhouXing19 a=ZhouXing19
As revealed by #84309, some keywords not added to `bare_label_keywords` in `sql.y` make the `SELECT ... <keyword>` statement error out, which is not compatible with postgres. This commit is to add the `QUERY` keyword per a support ticket. We're not adding the whole `unreserved_keyword` list here as having some of them in `bare_label_keyword`, such as `DAY`, brings `reduce` errors.
Postgres:
```
postgres=# select substring('stringstringstring',1,10) query;
query
------------
stringstri
(1 row)
```
CRDB:
```
root@localhost:26257/defaultdb> select substring('stringstringstring',1,10) query;
invalid syntax: statement ignored: at or near "query": syntax error
SQLSTATE: 42601
DETAIL: source SQL:
select substring('stringstringstring',1,10) query
^
```
informs #84309
Release Note(bug fix): fixed the syntax error for `SELECT ... QUERY` (without AS) statement.
97134: storage: include version in benchmark fixture directory r=RaduBerinde a=RaduBerinde
This benchmark is sometimes failing in CI and I cannot reproduce locally. The error is consistent with a leftover fixture from an older version (which shouldn't happen because CI should be cleaning up the repo).
This change adds the version to the fixture name so that this kind of cross-version problem cannot occur. This is a good idea regardless of the CI issue.
Informs #97061
Release note: None
Epic: none
97149: sql: add missing STORING clause in system.privileges user ID migration r=rafiss a=andyyang890
This patch fixes a discrepancy between the bootstrap schema and user ID
migration for the system.privileges table.
Part of #87079
Release note: None
Co-authored-by: Yevgeniy Miretskiy <[email protected]>
Co-authored-by: Jane Xing <[email protected]>
Co-authored-by: Radu Berinde <[email protected]>
Co-authored-by: Andy Yang <[email protected]>
|
Adding the version in the fixture name fixed this issue. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Some benchmarks in the CRDB repository generate fixtures in the package directory and reuse them if they already exist; these fixtures are .gitignored so they survive branch checkouts.
The reason for this is two-fold: re-running the benchmarks is much faster; but more importantly the results are more consistent since they run on exactly the same data.
There are two big problems:
Examples:
engineccl.BenchmarkTimeBoundIterate(currently disabled),batcheval.BenchmarkRefreshRange, many benchmarks instorage.We need to:
An example of CI failure is here: https://teamcity.cockroachdb.com/viewLog.html?buildId=8689881&tab=buildResultsDiv&buildTypeId=Cockroach_Ci_Tests_Bench
The panic is
"pebble: range keys require at least format major version .."which means that the store format is very old.CC @jbowens
Jira issue: CRDB-24486
The text was updated successfully, but these errors were encountered: