Note that while this change makes the snapshotting atomic, it doesn't
yet do anything to make sure containers are named at the same time that
they are added to the database. We can do that by adding a transactional
interface, either as a followup, or as part of this PR.

Thinking about this a little bit, usually the only name we care about is container.Name. The only case when we have other names are when links are in use, and I don't think there's any point in adding these transactionally. So maybe all we need to do here is automatically reserve container.Name when a container is added to the database. We're already automatically unregistering the name when the container is deleted.

Edit: Actually, there's no point in doing this, because we want to pre-reserve the name before we create the container. Otherwise, we might go through all the work of creating the container, and then find out the name is taken when we try to insert it into the memdb. So I think the PR makes sense as-is, and we just have to accept that some names in the DB might not have corresponding containers yet (see #33883).

Rebased and added test coverage. PTAL

I've added a new commit with the requested changes. I have mixed feelings about these changes. I think you are correct that adding these error checks and aborts is technically the right thing to do, but I'm concerned that it makes the code more complex and fragile. Since we can't use defer txn.Commit(), it will be much easier to leak the transaction lock in the future. I'm not sure there are any failure modes in practice that would cause Abort vs Commit to make a difference - the only paths that would allow us to commit a change partially seem to be internal memdb errors that shouldn't happen.

I'm not concerned about the lock contention. I think that's the price that has to be paid for keeping a consistent view of naming. All of these reservation and release operations should be trivial and only hold the lock very briefly.

(snip), but I'm concerned that it makes the code more complex and fragile.

This may be a bit more robust way to abort on errors, which will avoid leaking the transaction lock: https://play.golang.org/p/KGszH6RfPP

The error handling could be extracted into a func commitOrAbort(err error), or something similar, and be used as defer one-liners (defer commitOrAbort(retErr)) as well.

Alternatively, you can also go the functional route with:

func ...() {
	withTx(func(tx boltdb.Tx) error {
		// ...
	})
}

I've taken the functional approach.

LGTM

SGTM

Needs a rebase.

What do you think about also doing the same treatment for ID's, currently stored in a separate patricia-trie for prefix matching (as a separate PR).

Rebased.

What do you think about also doing the same treatment for ID's, currently stored in a separate patricia-trie for prefix matching (as a separate PR).

This would be an excellent way to support ID lookups. memdb uses a prefix tree internally and supports prefix matching for lookups. The IDs are already stored in the container objects, so there would be no ned to store anything else. We could just remove other redundant code.

LGTM