storageccl: use NewPebbleIterator in restore data processor #83984

msbutler · 2022-07-07T13:04:01Z

This PR replaces the multiIterator used in the restore data processor with the
PebbleSSTIterator, which has baked in range key support.

This patch is apart of a larger effort to teach backup and restore about MVCC
bulk operations. Next, the readAsOfIterator will need to learn how to
deal with range keys.

Informs #71155

Release note: none

cockroach-teamcity · 2022-07-07T13:04:14Z

This change is

Currently, if `pebble.NewExternalIter` sets pebbleIterator.inuse to True, but then fails, the subsequent `pebbleIterator.destroy()` will panic unecessarily, since the caller of `pebble.NewExternalIter` is not actually using the iter. This bug causes TestBackupRestoreChecksum to flake in cockroachdb#83984. To fix, this patch uses pebble.Close() to gracefully close the pebbleIterator if `pebble.NewExternalIter` fails. Release Note: None

83014: ui: add internal app filter to active statements and transactions pages r=ericharmeling a=ericharmeling This PR adds a single internal app filter option on to the Active Statements and Active Transactions pages. Active statements and transactions run by internal apps are no longer displayed by default. See commit message for release note. https://user-images.githubusercontent.com/27286675/174156635-39d8649a-df91-4550-adb5-b3c167d54ed5.mov Fixes #81072. 83707: roachtest: run workload from the tenant node r=knz a=stevendanna The secure URL refers to paths on disk on the clusters in the node. Since we only create the tenant-scoped certs on the tenant node, we need to run workload from that node. Fixes #82266 Depends on #83703 Release note: None 84003: storage: close pebble iter gracefully when NewPebbleSSTIterator fails r=erikgrinaker a=msbutler Currently, if `pebble.NewExternalIter` sets pebbleIterator.inuse to True, but then fails, the subsequent `pebbleIterator.destroy()` will panic unecessarily, since the caller of `pebble.NewExternalIter` is not actually using the iter. This bug causes TestBackupRestoreChecksum to flake in #83984. To fix, this patch uses pebble.Close() to gracefully close the pebbleIterator if `pebble.NewExternalIter` fails. Release Note: None 84039: prometheus: use older node_exporter r=nicktrav a=tbg v1.3.1, the most up to date released version, has a bug that inflates the bytes written by ~8x for NVMe drives (which in particular includes the default drives for our GCE roachprod machines). Fundamentally this is caused by the fact that these devices use a 4K sector size whereas the kernel will always report based on a 512B sector size. This took us a while to figure out, and to avoid repeating this exercise periodically, downgrade node_exporter to 1.2.2, which pre-dates a refactor that introduces the regression. See: prometheus/node_exporter#2310 Release note: None Co-authored-by: Eric Harmeling <[email protected]> Co-authored-by: Steven Danna <[email protected]> Co-authored-by: Michael Butler <[email protected]> Co-authored-by: Tobias Grieger <[email protected]>

erikgrinaker

The storage parts LGTM.

I haven't looked into how this currently works on the bulk side, but I'm assuming that we're already pulling these into memory, so it isn't problematic that we do so here?

Also, note that the new SST iterators are significantly slower than the previous ones (~80%), so we should benchmark this once everything is hooked up and look into optimizations.

msbutler · 2022-07-08T11:29:43Z

I haven't looked into how this currently works on the bulk side, but I'm assuming that we're already pulling these into memory, so it isn't problematic that we do so here?

Previously, ExternalSSTReader() with kv.bulk_ingest.stream_external_ssts.enabled set to true (by default), would try not to load the whole SST into memory via storage.NewSSTIterator(), which gets passed a storageccl.sstReader, an implementation of pebble's sstable.ReadableFile interface. If this cluster setting is disabled, the whole SST would get loaded into memory, via storage.NewMemSSTIterator().

I'm attempting to follow the same pattern during setup:

If kv.bulk_ingest.stream_external_ssts.enabled is set to true, I buffer a bunch of storageccl.sstReaders that prevent a full read into memory, and pass them to NewPebbleSSTIterator
if the setting is false, I load all the SSTs to memory, and pass them to NewPebbleMultiMemSSTIterator

Currently during restore processing, the multiplexed iterator will iterate over a keySpan that each SST in the iterator has some overlap with. Note that the file's span could be much larger than the key span we seek to iterate over, so I don't think the whole file is necessarily iterated through nor loaded all into memory. (@dt please correct me if I'm wrong here).

Are you implying that NewPebbleSSTIterator will have to load all its SSTs into memory, unlike (I think) the multIterator we use now?

erikgrinaker · 2022-07-08T11:46:51Z

Are you implying that NewPebbleSSTIterator will have to load all its SSTs into memory, unlike (I think) the multIterator we use now?

A question for you: Are you implying above that NewPebbleSSTIterator will load all the SSTs into memory anyway?

Nah, I was just skimming this and saw the use of NewExternalMemPebbleSSTReader. The SST iterator will use the provided reader, so if that's on disk then it'll read from disk.

pkg/ccl/backupccl/restore_data_processor.go

pkg/ccl/storageccl/external_sst_reader.go

This PR replaces the multiIterator used in the restore data processor with the PebbleSSTIterator, which has baked in range key support. This patch is apart of a larger effort to teach backup and restore about MVCC bulk operations. Next, the readAsOfIterator will need to learn how to deal with range keys. This PR leaves some clean up work to remove all calls of DeprecatingExternalSSTReader. Informs cockroachdb#71155 Release note: none

msbutler · 2022-07-11T19:08:37Z

TFTRs!!

bors r=erikgrinaker

craig · 2022-07-11T21:03:18Z

Build succeeded:

GitHub CI (Cockroach)

This PR refactors all call sites of ExternalSSTReader(), to support using the new PebbleIterator, which has baked in range key support. Most notably, this PR replaces the multiIterator used in the restore data processor with the PebbleSSTIterator. This patch is apart of a larger effort to teach backup and restore about MVCC bulk operations. Next, the readAsOfIterator will need to learn how to deal with range keys. Informs cockroachdb#71155 This PR addresses a bug created in cockroachdb#83984: loop variables in ExternalSSTReader were captured by reference, leading to roachtest failures (cockroachdb#84240, cockroachdb#84162). Informs #71155i Fixes: cockroachdb#84240, cockroachdb#84162, cockroachdb#84181 Release note: none

84452: sql: add troubleshooting mode session variable r=THardy98 a=THardy98 Resolves: #84429 This change introduces a `troubleshooting_mode_enabled` session variable. When enabled, this session variable is intended to be used as a way to avoid performing additional work on queries, particularly when the cluster is experiencing issues/unavailability/failure. By default, this session variable is disabled. Currently, this session variable is only used to avoid collecting/emitting telemetry data. Release note (sql change): Introduce new `troubleshooting_mode_enabled` session variable, to avoid doing additional work on queries when possible (i.e. collection telemetry data). By default, this session variable is disabled. 84666: storageccl: use the new PebbleIterator in ExternalSSTReader r=erikgrinaker a=msbutler This PR refactors all call sites of ExternalSSTReader(), to support using the new PebbleIterator, which has baked in range key support. Most notably, this PR replaces the multiIterator used in the restore data processor with the PebbleSSTIterator. This patch is apart of a larger effort to teach backup and restore about MVCC bulk operations. Next, the readAsOfIterator will need to learn how to deal with range keys. Informs #71155 This PR addresses a bug created in #83984: loop variables in ExternalSSTReader were captured by reference, leading to roachtest failures (#84240, #84162). Informs #71155 Fixes: #84240, #84162, #84181 Release note: none Co-authored-by: Thomas Hardy <[email protected]> Co-authored-by: Michael Butler <[email protected]>

msbutler force-pushed the butler-bulk-ops branch from 6dce069 to 92a2ddf Compare July 7, 2022 14:47

msbutler self-assigned this Jul 7, 2022

msbutler mentioned this pull request Jul 7, 2022

storage: close pebble iter gracefully when NewPebbleSSTIterator fails #84003

Merged

msbutler force-pushed the butler-bulk-ops branch from 92a2ddf to 2346839 Compare July 8, 2022 00:02

msbutler requested review from erikgrinaker and dt July 8, 2022 01:51

msbutler marked this pull request as ready for review July 8, 2022 01:51

msbutler requested a review from a team as a code owner July 8, 2022 01:51

msbutler requested review from a team and removed request for a team July 8, 2022 01:51

msbutler force-pushed the butler-bulk-ops branch from 2346839 to 9abb4d9 Compare July 8, 2022 03:21

erikgrinaker approved these changes Jul 8, 2022

View reviewed changes

msbutler force-pushed the butler-bulk-ops branch from 9abb4d9 to f05262e Compare July 8, 2022 11:05

dt reviewed Jul 10, 2022

View reviewed changes

pkg/ccl/backupccl/restore_data_processor.go Outdated Show resolved Hide resolved

pkg/ccl/storageccl/external_sst_reader.go Outdated Show resolved Hide resolved

msbutler force-pushed the butler-bulk-ops branch from f05262e to 9c8eeda Compare July 10, 2022 13:59

msbutler force-pushed the butler-bulk-ops branch from 9c8eeda to 4d1f66e Compare July 11, 2022 15:45

msbutler requested a review from a team as a code owner July 11, 2022 15:45

msbutler mentioned this pull request Jul 11, 2022

backupccl: remove all calls of DeprecatingExternalSSTReader #84181

Closed

craig bot merged commit 10853d2 into cockroachdb:master Jul 11, 2022

msbutler deleted the butler-bulk-ops branch July 11, 2022 21:06

This was referenced Jul 12, 2022

roachtest: backupTPCC failed #84240

Closed

storage: pebbleIterator panics too eagerly on Close() #84396

Closed

msbutler mentioned this pull request Jul 19, 2022

storageccl: use the new PebbleIterator in ExternalSSTReader #84666

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

storageccl: use NewPebbleIterator in restore data processor #83984

storageccl: use NewPebbleIterator in restore data processor #83984

msbutler commented Jul 7, 2022

cockroach-teamcity commented Jul 7, 2022

erikgrinaker left a comment

msbutler commented Jul 8, 2022 •

edited

Loading

erikgrinaker commented Jul 8, 2022

msbutler commented Jul 11, 2022

craig bot commented Jul 11, 2022

storageccl: use NewPebbleIterator in restore data processor #83984

storageccl: use NewPebbleIterator in restore data processor #83984

Conversation

msbutler commented Jul 7, 2022

cockroach-teamcity commented Jul 7, 2022

erikgrinaker left a comment

Choose a reason for hiding this comment

msbutler commented Jul 8, 2022 • edited Loading

erikgrinaker commented Jul 8, 2022

msbutler commented Jul 11, 2022

craig bot commented Jul 11, 2022

msbutler commented Jul 8, 2022 •

edited

Loading