[itsbilal]

This change updates the comment at the function signature of
db.Ingest to highlight that all callers to that method need to
ensure all writes to sstables are synced to disk, to guarantee
durability of ingested data.

Fixes #787.

[cockroach-teamcity]

This change is 

[petermattis]

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @itsbilal, @jbowens, and @petermattis)

---

ingest.go, line 508 at r1 (raw file):

	// files before calling Ingest, but do it again just in case.
	for _, path := range paths {
		f, err := d.opts.FS.Open(path)

Is it kosher to call Sync on a file opened read-only? Does the Sync actually do anything?

---

ingest.go, line 513 at r1 (raw file):

		}
		if err := f.Sync(); err != nil {
			return err

This leaks an open file descriptor on error. You might want to use firstError here:

err := f.Sync()
err = firstError(err, f.Close())
if err != nil { ... }

[petermattis]

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @itsbilal, @jbowens, and @petermattis)

---

ingest.go, line 508 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Is it kosher to call Sync on a file opened read-only? Does the Sync actually do anything?

The internets are not giving me a definitive answer here. Apparently this works for directories under linux. Not clear about files. Not clear about whether this works on Windows. Sigh.

[itsbilal]

Did some research on the interwebs, and it seems like fsync on some versions of linux often means "flush all bytes since last successful fsync on this file handle". Behaviour with Linux also seems FS dependent. No idea about Windows. The standard seems to advise write-sync-close as opposed to write-close-reopen-sync, as the former is a better durability guarantee.

I'm leaning towards just relying on the callers to call Sync. What do you think?

Postgres' mailing list thread on this: https://www.postgresql.org/message-id/flat/CAMsr%2BYHh%2B5Oq4xziwwoEfhoTZgr07vdGG%2Bhu%3D1adXx59aTeaoQ%40mail.gmail.com#CAMsr+YHh+5Oq4xziwwoEfhoTZgr07vdGG+hu=1adXx59aTeaoQ@mail.gmail.com
Postgres wiki entry on errored fsyncs and retrying them: https://wiki.postgresql.org/wiki/Fsync_Errors
Stack Overflow question: https://stackoverflow.com/questions/37288453/calling-fsync2-after-close2
Linux-fsdevel mailing list: https://marc.info/?l=linux-fsdevel&m=152535409207496&w=2

[petermattis]

I'm leaning towards just relying on the callers to call Sync. What do you think?

Based on these docs, I think that sounds reasonable. I think we should enhance the comment on DB.Ingest to make this requirement clear. Include these discussion links so that our future self can easily find them.

[itsbilal]

Done. This is now a comment-only PR.

[petermattis]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @itsbilal and @jbowens)

[itsbilal]

TFTR!