Add Blob I/O

[joriszwart]

Incremental Blob I/O

This PR adds support for Incremental Blob I/O.

Notes

• Modeled after backup using a driver connection.
• The interfaces io.Reader and io.Closer have been implemented.
• Related to issue Blob I/O #239

If this PR makes sense, I'll enhance it with additional io interfaces.

[mattn]

interesting.

[joriszwart]

• I guess it depends on Fix test queryer test #1079 to have successful checks.

[joriszwart]

I have implemented write and seek support. As far as I'm concerned, this pull request is ready for further review. Thanks so far.

[joriszwart]

I was tempted to implement the io.ReaderAt and io.WriterAt interfaces as well, but clients can do that themselves by combining io.Reader (or io.Writer) and io.Seeker:

type Foo struct {
	io.ReadSeeker
}

func (f *Foo) ReadAt(p []byte, off int64) (int, error) {
	_, err := f.Seek(off, io.SeekStart)
	if err != nil {
		return 0, err
	}

	n, err := f.Read(p)
	return n, err
}

[jlelse]

Any updates on this? It looks very interesting and enables "streaming" to and from the database! 👍

[joriszwart]

@rittneje Is there anything I can do to get this approved?

[rittneje]

Trying to remember - is there a reason not to do this check after the call to sqlite3_blob_write and only write what we can instead of nothing? I guess the current implementation is consistent with what sqlite3_blob_write internally does.

[rittneje]

@joriszwart following up on this

[joriszwart]

I don't know what to do. Sorry.

[joriszwart]

@rittneje can you help me out?

[gabriel-samfira]

This function implements io.Writer{}, which states:

https://pkg.go.dev/io#Writer

Write writes len(p) bytes from p to the underlying data stream. It returns the number of bytes written from p (0 <= n <= len(p)) and any error encountered that caused the write to stop early. Write must return a non-nil error if it returns n < len(p). Write must not modify the slice data, even temporarily.

The interesting bit of this is:

Write must return a non-nil error if it returns n < len(p).

So if b is partially written, then n should return the number of bytes written and a non nil error. But to be honest, the way it's written now, should be fine. I mean, partial writes are worse than no writes. At least the caller knows it erred and can retry using the same byte slice.

At least that's what I understand from the io.Writer docs.

[gabriel-samfira]

The only thing that would be nice to have is maybe an error that denotes not enough space. Something like ENOSPC on linux and ERROR_HANDLE_DISK_FULL on Windows (or if sqlite already has an error code for not enough space, to use that). But that's just a nit. It would help the caller when checking with errors.Is(err, NotEnoughSpacePlatformSpecificError).

I think sqlite has SQLITE_FULL and SQLITE_IOERR_DISKFULL.

[rittneje]

partial writes are worse than no writes

This is a subjective statement.

At least the caller knows it erred and can retry using the same byte slice.

If the error is because the blob is full, retrying is never going to work.

[gabriel-samfira]

You're right on both counts.

[rittneje]

@joriszwart I am away from my dev machine or I'd just add those two len checks myself. They should be pretty simple to add.

[joriszwart]

@joriszwart I am away from my dev machine or I'd just add those two len checks myself. They should be pretty simple to add.

Can you add them?

[joriszwart]

Anyone else? @graf0 @pokstad @jlelse @lezhnev74 @mitar?

[rittneje]

Remove. This is superfluous with the call to conn.Close() above. (And also you aren't supposed to do anything with the conn outside the Raw callback.)

[joriszwart]

Remove those 4 lines? Or only the last?

[joriszwart]

Remove only the last line?

[gabriel-samfira]

I think just line 72. The driverConn is just conn cast as the underlying type that implements the interface (conn). Calling Close() on conn will close driverConn as well.

[joriszwart]

@rittneje can I leave the suggested changes to you? That would make this process more efficient.

[gabriel-samfira]

A shame this was abandoned. Thanks for trying @joriszwart !

[joriszwart]

@mattn @rittneje any interest in this?

[gabriel-samfira]

@mattn @rittneje any interest in this?

for what it's worth, I would love to see this happen.

[gabriel-samfira]

I recommend you create a copy of b and use that throughout. Slices may be mutated after passed to Write(). You might end up writing the first few bytes from the initial data and the rest from the new set when b is overwritten by the caller. In go only the slice header is passed by value. The underlying array is shared with the caller.

Something like:

tmp := make([]byte, len(b))
copy(tmp, b)

Then use tmp instead of b.

[joriszwart]

Doesn't making a copy defeat the purpose of streaming blobs?

[gabriel-samfira]

Not really, as you'd only be copying the buffer (which is small - usually around 1024 bytes). But generally speaking you'd only have to copy if you need to guard against caller reuse of the buffer. This is usually done in logging writers that may want to return control back to the caller while the write operation against the logging backend is still ongoing.

I'm not sure that would apply here though (given that we want to wait for the write to happen and only then return), so I just added a comment as a "recommendation". Feel free to ignore me.

I haven't mentioned this, but thanks for reopening this PR!

[rittneje]

There is no need to make a copy of the slice, as sqlite3_blob_write is going to copy the data. And the caller is not allowed to modify the slice during the call the Write. (Even if they did, copy itself would be subject to a race condition anyway.)

[gabriel-samfira]

for what it's worth, I rebased this PR on top of main and I'm using it in one of my projects. It works great. This actively makes my life easier, as I don't need to add an additional requirement for users to configure a new writable path for small (very limited in number and size) files that the app requires to function properly. So thanks @joriszwart for this PR.

[joriszwart]

So, how to proceed?

[mattn]

Thank you for this PR. The feature itself is useful and we'd like to get it merged. However, there are a few things that need to be addressed first:

• Needs a rebase onto current master (the base is from 2022)
• The len checks mentioned by @rittneje are still missing
• lastError() signature has changed — calls need to be updated
• The test uses :memory: with SetMaxOpenConns(1) — please use file:/<name>?vfs=memdb instead