storage/engine,libroach: 1x write amplification on ingest sst

[dt]

This uses the new flag added in facebook/rocksdb#4172 to avoid needing to make global_seq_no-related edits to SSTs, and thus avoid the SST copying those edits forced us to do to preserve the raft log immutability when hard-linking directly to side load SSTs.

This is gated on a cluster version that is defined well after the upgrade to rocks 5.17, since using the flag requires that no older versions of RocksDB (<5.16),
will ever be used to read these SSTs, as they will lack the seq_no that the older versions need for correctness.

Release note (performance improvement): reduce write-amplification during bulk-loading (IMPORT and RESTORE)

[cockroach-teamcity]

This change is 

[petermattis]

Were you able to measure a perf benefit or otherwise verify that this reduces disk traffic during IMPORT? Or perhaps I'd be satisfied if we could have a unit test that this is working properly. I'm not quite sure how that would work, but perhaps generate an SST and ingest it and verify that the SST is unmodified. Somehow. Maybe.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @dt, @petermattis, and @tbg)

---

c-deps/libroach/db.cc, line 700 at r1 (raw file):

  // only safe to disable the seqno if older versions of RocksDB (<5.16) will
  // not be used to read these SSTs.
  // Before editing a file to insert a seq_no, RocksDB also checks the option

Nit: add a blank line between paragraphs.

[dt]

Realized it wasn't working because rocks checks the allow_modification flag before it checks if it is actually using seqno at all. Added a || skip_seqno to the allow_modification flag indeed, it seems to be working as expected. On a very fast MBP SSD that is otherwise unloaded, the speedup on a direct (overlapping) import of ~1.7gb of tpcc data looks like about 5%, but past experience is that using available disk IO is a bigger deal on more throughput constrained disks (e.g. ebs) or with other traffic contending for disk io. I observed the process wrote about 1.5gb less, but with overlapping import, the heavy recompactions dominate total write traffic so it only worked out to about a 20% reduction in total write traffic.

[dt]

@petermattis turns out we did actually have a unit test this that should have started failing if the original change had worked as intended -- I found (and updated it) when I fixed it. Here's the sst apply vs copy with and without this change enabled:

[petermattis]

Looks good. Minor nit remaining.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @dt and @tbg)

---

c-deps/libroach/db.cc, line 700 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Nit: add a blank line between paragraphs.

Ping.

---

c-deps/libroach/include/libroach.h, line 412 at r2 (raw file):

// it would have tried to modify any of the files' sequence numbers rather than
// editing the files in place. If skip_seqno is true, it will skip the
// seq_no entirely -- this is only safe if this will only ever be read by Rocks

This is a bit confusing. skip_seqno indicates that the global seqno doesn't need to be written into the sstable. The global seqno is written into the LSM state, just outside of the sstable. I'd prefer to keep this parameter named as write_global_seqno, and to clarify the commentary.

[dt]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @petermattis and @tbg)

---

c-deps/libroach/db.cc, line 700 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Ping.

Done.

---

c-deps/libroach/include/libroach.h, line 412 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

This is a bit confusing. skip_seqno indicates that the global seqno doesn't need to be written into the sstable. The global seqno is written into the LSM state, just outside of the sstable. I'd prefer to keep this parameter named as write_global_seqno, and to clarify the commentary.

Done.

[dt]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)