Table store #774

cody-littley · 2024-09-24T22:02:29Z

Why are these changes needed?

This PR adds the ability to use tables on top of a store. A very nice to have feature for use cases that want to use multiple logical tables.

Checks

I've made sure the lint is passing in this PR.
I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
I've checked the new test coverage and the coverage percentage didn't drop.
Testing Strategy
- Unit tests
- Integration tests
- This PR is not tested :(

Signed-off-by: Cody Littley <[email protected]>

jianoaix · 2024-10-02T18:17:19Z

common/kvstore/batch.go

+package kvstore
+
+// Batch is a collection of operations that can be applied atomically to a TableStore.
+type Batch[T any] interface {


Not sure this needs template, Batch may just work with table key

Base Store objects also need to support operations using Batch, but the type of these keys is []byte. I think if we want to get rid of the generics, we will need to stop using type TableKey []byte and instead use the []byte type for batch operations on a table. Let's discuss.

The issue is this interface intends to be used for TableStore (like it comments out), but it's now got used to batch operations for Store as well.

One idea is to subclass this interface with two types, like (demo not real code):

type StoreBatch struct { // key type is []byte deletes map[[]byte]struct{} ... } type TableStoreBatch struct { // key type is TableKey deletes map[TableKey]struct{} ... }

It establishes common interface but users doesn't need to use template. Wdyt?

I'm ok with this approach. If we do it this way, I'd actually be in favor of ditching the shared interface, and just embrace the fact that batches against multiple tables have a different API.

I'll wait to make this change until we figure out what we are doing with thread safety. With my temporary branch currently open, I don't want to start introducing merge conflicts. 🙃

It's a good pattern to have a Base<T> and then subclass it with CaseA and CaseB without template. The interface helps constrain what they can be.

jianoaix · 2024-10-02T18:17:47Z

common/kvstore/batch.go

+}
+
+// BatchOperator is an interface for creating new batches.
+type BatchOperator[T any] interface {


For simplicity, it looks Batch creation can be a function of TableStore

NewBatch is also a function on the base Store object, but I agree this interface isn't really necessary. Removed.

jianoaix · 2024-10-02T18:27:39Z

common/kvstore/tablestore/table_store_wrapper.go

+	"sort"
+)
+
+// Table ID 0 is reserved for use internal use by the metadata table.


Comment doesn't match code

jianoaix · 2024-10-02T18:27:43Z

common/kvstore/tablestore/table_store_wrapper.go

+// Table ID 0 is reserved for use internal use by the metadata table.
+const metadataTableID uint32 = math.MaxUint32
+
+// Table ID 1 is reserved for use by the namespace table. This stores a mapping between IDs and table names.


Comment doesn't match code

jianoaix · 2024-10-02T18:30:09Z

common/kvstore/tablestore/table_store_wrapper.go

+// WARNING: it is not safe to access the wrapped store directly while the TableStore is in use. The TableStore uses
+// special key formatting, and direct access to the wrapped store may violate the TableStore's invariants, resulting
+// in undefined behavior.
+func Wrapper(logger logging.Logger, base kvstore.Store) (kvstore.TableStore, error) {


I'd create a TableStore, instead of asking for an external Store and wrapping it. This way the Store can be internally created and no risk of breaking invariants by accessing it directly outside the TableStore.

The purpose of requiring the caller to provide the Store implementation is to support multiple Store implementations. Currently this is used in unit tests, where I sometimes use a store implementation that is built on top of an in memory map. Let's discuss this.

Changed based on our slack discussion.

jianoaix · 2024-10-02T18:32:47Z

common/kvstore/tablestore/table_store_wrapper.go

+}
+
+// GetTable gets the table with the given name. If the table does not exist, it is first created.
+func (t *tableStore) GetTable(name string) (kvstore.Table, error) {


I don't see this TableStore is thread-safe (which is required per interface)

In the interface, this method is specifically documented not to be thread safe (although the godoc in the implementation doesn't copy the warning). The code to make this thread simple to write, but I'm concerned about adding locks that could slow other operations. Do you think it's worth adding locks so that these methods are safe to call concurrently?

// GetTable gets the table with the given name. If the table does not exist, it is first created. // // WARNING: this method is not thread safe with respect to any other methods in this interface or // any methods on any Table objects associated with this store. GetTable(name string) (Table, error)

I think it needs to, otherwise other methods like GetTableCount (which is supposed to be thread safe) won't be safe as they share the states.

There are now no unsafe methods (due to schema pattern)

Signed-off-by: Cody Littley <[email protected]>

jianoaix · 2024-10-09T02:02:26Z

common/kvstore/tablestore/table_store_builder.go

+	schemaKey := []byte(metadataSchemaVersionKey)
+	onDiskSchemaBytes, err := metadataTable.Get(schemaKey)
+
+	if errors.Is(err, kvstore.ErrNotFound) {


The control below can be made more readable:

if err != nil { if !errors.Is(err, kvstore.ErrNotFound) { return ... } // handle not found case ... return nil } // Verify schema version ...

jianoaix · 2024-10-09T02:03:55Z

common/kvstore/tablestore/table_store_builder.go

+		// Verify schema version.
+		onDiskSchema := binary.BigEndian.Uint64(onDiskSchemaBytes)
+		if onDiskSchema != currentSchemaVersion {
+			// In the future if we change schema versions, we may need to write migration code here.


How do you migrate? The data will need to be re-processed.

Most likely, a migration will require us to iterate all data in the DB and to change its on disk format. The schema version is included here as a "just in case" sort of thing. I REALLY hope we never have to do this, so we should do our best to design the schema right the first time.

jianoaix · 2024-10-09T02:07:08Z

common/kvstore/tablestore/table_view.go

+
+var _ kvstore.Table = &tableView{}
+
+// tableView allows table in a New to be accessed as if it were a Store.


The comment not readable/has typo ("in a New"?, "as it were a Store"?)

Fixed.

// tableView allows data in a table to be accessed as if it were a Store.

This is an artifact of my IDE attempting to automatically refactor things when I changed the name of the method.

jianoaix · 2024-10-09T02:13:24Z

common/kvstore/tablestore/table_store_builder.go

+	"sort"
+)
+
+// The table ID reserved for the metadata table.


Comment what's in the metadata table (like what you did for namespaceTable)

// The table ID reserved for the metadata table. The metadata table is used for internal bookkeeping. // The following data is currently stored in the metadata table: // - Schema version (in case we ever need to do a schema migration) // - Table deletion markers used to detect a crash during table deletion and to complete // the deletion when the store is next started.

jianoaix · 2024-10-09T02:17:56Z

common/kvstore/tablestore/table_store_builder.go

+// This method will set up a table for each table name provided, and will drop all tables not in the list.
+// Dropping a table is irreversible and will delete all data in the table, so be very careful not to call
+// this method with table names omitted by mistake.
+func (t StoreType) Create(logger logging.Logger, path string, tables ...string) (kvstore.TableStore, error) {


It looks a more frequent operation will be Load, where it's given the path, and will load the existing base Store as well as existing tables.

I've renamed this to Start(), since that name is accurate for both a new setup and for when we load existing data.

Potential UX issues to check: 1) the users mostly may just want to load existing tables (adding/dropping tables are rare operations), and 2) the user may not know the existing tables, unless they can build this TableStore, which then requires knowing the existing tables (a loop here)

I added a method Load() which can be used to load the store without making any modifications to the schema.

Signed-off-by: Cody Littley <[email protected]>

jianoaix · 2024-10-10T03:57:25Z

common/kvstore/tablestore/table_store_builder.go

+		return nil, fmt.Errorf("error handling incomplete deletion: %w", err)
+	}
+
+	err = addAndRemoveTables(base, metadataTable, namespaceTable, tableIDMap, tables)


I'd prefer not deleting tables synchronously. After the database running for a while, the deletion can take quite a while. It'd be better to:

Do lazy deletion: write the table to be deleted to the deletion table

Run a thread in the background to perform the deletion - and cap the resource it can use

We discussed this, repeating it here for the sake of others. This PR is already fairly large, and so we've decided to implement async table deletion as a follow up PR.

jianoaix · 2024-10-10T03:58:42Z

common/kvstore/tablestore/table_store_builder.go

+// The table ID reserved for the metadata table. The metadata table is used for internal bookkeeping.
+// The following data is currently stored in the metadata table:
+//   - Schema version (in case we ever need to do a schema migration)
+//   - Table deletion markers used to detect a crash during table deletion and to complete


If you adopt lazy deletion, then this can be a separate DeletionTable, which records each deleted table as an entry in this table. The background thread work on the DeletionTable and perform GC.

Signed-off-by: Cody Littley <[email protected]>

jianoaix

Almost there!

jianoaix · 2024-10-10T19:56:28Z

common/kvstore/tablestore/table_store_builder.go

+	currentTables []string) error {
+
+	// Determine which tables to keep and which to drop.
+	originalTablesSet := make(map[string]bool)


Use map[string]struct{} for implementing set will use slightly less memory

Done. One of the things that gets on my nerves with golang is the absence of a set in the standard libraries. Sure, you can mimic it with a map. It's just syntactically ugly.

jianoaix · 2024-10-10T20:02:44Z

common/kvstore/tablestore/table_store_builder.go

+
+// This method adds and removes tables as needed to match the given list of tables. The table ID map is updated
+// to reflect the new state of the tables.
+func addAndRemoveTables(


Potentially more readability if we build this into:

computeSchemaChanges: returns tablesToAdd and tablesToDrop

addTables(tablesToAdd)

dropTables(tablesToDrop)

jianoaix · 2024-10-10T20:04:01Z

common/kvstore/tablestore/table_store_builder.go

+
+// Perform sanity checks on the namespace table.
+// This method makes potential logic in the namespace errors fail fast and visibly.
+func sanityCheckNamespaceTable(


Is this for testing/debugging the implementation is correct?

This is sanity check code intended to make noise if a bug in the code results in inconsistencies between tableIDMap, namespaceTable, and the user provided schema. I intended this check to run even in production environments, since if the tables are corrupted we will have really big problems.

That being said, if you don't think it belongs I'm willing to remove it.

Sounds fine to keep then

jianoaix · 2024-10-10T20:08:53Z

common/kvstore/tablestore/table_store_builder.go

+		return nil, fmt.Errorf("error loading namespace table: %w", err)
+	}
+
+	err = handleIncompleteDeletion(logger, base, metadataTable, namespaceTable)


Shouldn't this deletion happen before even loading the namespace table? Otherwise a name/ID that's still pending for deletion may get reused

You are correct that the partially deleted table may already be inside the namespace table. This edge case is handled by handleIncompleteDeletion(), which will perform the necessary deletion within the namespace table (last line of handleIncompleteDeletion() calls dropTable(), which deletes the entry from the namespace table).

If there is a pending deletion, the loadNamespaceTable will include that deleted table in the returned tableIDMap (because that table may be still in the namespace table, haven't fully deleted), which is not desired

I walked this code with a debugger. It turns out this was accidentally working, since the method addAndRemoveTables() deletes the spurious table ID. There is a unit test that is sensitive to spurious entries in the table ID map, and that was not failing.

In order to make this more understandable and less easy to accidentally break, I've moved the loading of the table ID map until after handleIncompleteDeletion().

jianoaix · 2024-10-10T20:10:21Z

common/kvstore/tablestore/table_store_builder.go

+
+// The table ID reserved for the namespace table. Keys in the namespace table are table IDs (uint32)
+// and values are table names (string).
+const namespaceTableID uint32 = math.MaxUint32 - 1


It's helpful to call out in comment that table name and ID can be reused after a table is completely dropped from the TableStore.

// The table ID reserved for the namespace table. Keys in the namespace table are table IDs (uint32) // and values are table names (string). Although no two tables are permitted to have share a name or a table ID, // once a table is dropped, future tables may be instantiated with the same name and the table ID may be reused. const namespaceTableID uint32 = math.MaxUint32 - 1

jianoaix · 2024-10-10T20:12:52Z

common/kvstore/tablestore/table_store_builder.go

+	// We want to fill gaps prior to assigning new IDs.
+	newTableIDs := make([]uint32, 0, len(tablesToAdd))
+	nextID := uint32(0)
+	for i := 0; i < len(tablesToAdd); i++ {


Slightly simpler/more readable way to express this code:

for len(newTableIDs) < len(tablesToAdd) { if _, alreadyUsed := tableIDMap[nextID]; !alreadyUsed { newTableIDs = append(newTableIDs, nextID) } nextID++ }

Yes, that's a lot cleaner. Done.

Signed-off-by: Cody Littley <[email protected]>

jianoaix · 2024-10-15T15:33:10Z

common/kvstore/Batch.go

@@ -0,0 +1,14 @@
+package kvstore
+
+// Batch is a collection of key / value pairs that can be written atomically to a database.


nit: can be written -> will be written atomically?

Mention that the Batch is not thread safe

Oops, noticed this after I pressed the merge button. I will include this change in the follow up PR I am working on right now.

cody-littley added 13 commits September 19, 2024 15:35

Created table store.

c939ace

Signed-off-by: Cody Littley <[email protected]>

Incremental progress.

8889ea2

Signed-off-by: Cody Littley <[email protected]>

Simplify by making max table count fixed.

967b01c

Signed-off-by: Cody Littley <[email protected]>

Use last table indices for internal tables.

3f65d47

Signed-off-by: Cody Littley <[email protected]>

Make table deletion atomic.

8894e43

Signed-off-by: Cody Littley <[email protected]>

Cleanup.

61d8407

Signed-off-by: Cody Littley <[email protected]>

Added table view.

58d321c

Signed-off-by: Cody Littley <[email protected]>

Fix table view iteration.

a85f0ee

Signed-off-by: Cody Littley <[email protected]>

Write some unit tests.

4a37778

Signed-off-by: Cody Littley <[email protected]>

checkin

4ebe6d0

Signed-off-by: Cody Littley <[email protected]>

Add random test, fix bugs.

2695d49

Signed-off-by: Cody Littley <[email protected]>

Added logging.

b288669

Signed-off-by: Cody Littley <[email protected]>

Fix lint issues.

0913aff

Signed-off-by: Cody Littley <[email protected]>

cody-littley self-assigned this Sep 24, 2024

cody-littley added 11 commits September 25, 2024 08:09

lint

fc4b707

Signed-off-by: Cody Littley <[email protected]>

Checkin.

27d7f0b

Signed-off-by: Cody Littley <[email protected]>

Small tweak to naming.

98bdde9

Signed-off-by: Cody Littley <[email protected]>

Finished conversion, now for the unit tests

5bed795

Signed-off-by: Cody Littley <[email protected]>

Partially migrate to new API.

b6cd49b

Signed-off-by: Cody Littley <[email protected]>

Now compiles.

efc7be6

Signed-off-by: Cody Littley <[email protected]>

Get store tests working.

0dc38c2

Signed-off-by: Cody Littley <[email protected]>

Unit tests are passing.

a11e3b4

Signed-off-by: Cody Littley <[email protected]>

Cleanup.

dfe422a

Signed-off-by: Cody Littley <[email protected]>

Cleanup and unit tests.

091e6bb

Signed-off-by: Cody Littley <[email protected]>

Cleanup.

39a3521

Signed-off-by: Cody Littley <[email protected]>

cody-littley marked this pull request as ready for review September 30, 2024 19:38

cody-littley requested review from jianoaix and ian-shim October 1, 2024 13:26

jianoaix reviewed Oct 2, 2024

View reviewed changes

Made suggested changes.

3921d4a

Signed-off-by: Cody Littley <[email protected]>

cody-littley added 13 commits October 4, 2024 08:41

Made suggested changes.

ca76d4c

Signed-off-by: Cody Littley <[email protected]>

Refactored builder, need to convert unit tests

f424892

Signed-off-by: Cody Littley <[email protected]>

Things compile, many tests commented out

6d0c2b5

Signed-off-by: Cody Littley <[email protected]>

Enable more unit tests.

551e6cf

Signed-off-by: Cody Littley <[email protected]>

Enabled remainder of tests.

2117cd6

Signed-off-by: Cody Littley <[email protected]>

Partial conversion, compiles

d0c3654

Signed-off-by: Cody Littley <[email protected]>

Enabled most unit tests.

1a3c9ec

Signed-off-by: Cody Littley <[email protected]>

Incremental progress.

2a287ea

Signed-off-by: Cody Littley <[email protected]>

Start shuffling things.

0d90c45

Signed-off-by: Cody Littley <[email protected]>

Re-enable last commented test.

c2a29c0

Signed-off-by: Cody Littley <[email protected]>

More refactors.

eef049f

Signed-off-by: Cody Littley <[email protected]>

Finish simplifying builder.

5302e68

Signed-off-by: Cody Littley <[email protected]>

Made suggested changes, cleanup

b16a83f

Signed-off-by: Cody Littley <[email protected]>

jianoaix reviewed Oct 9, 2024

View reviewed changes

cody-littley added 2 commits October 9, 2024 09:01

Made suggested changes.

1dbafb5

Signed-off-by: Cody Littley <[email protected]>

Made suggested change.

473e483

Signed-off-by: Cody Littley <[email protected]>

jianoaix reviewed Oct 10, 2024

View reviewed changes

Add ability to load store without changing tables.

dcd087e

Signed-off-by: Cody Littley <[email protected]>

jianoaix reviewed Oct 10, 2024

View reviewed changes

cody-littley added 2 commits October 10, 2024 15:53

Made suggested changes.

553b6c4

Signed-off-by: Cody Littley <[email protected]>

Make suggested change.

2780af4

Signed-off-by: Cody Littley <[email protected]>

jianoaix approved these changes Oct 15, 2024

View reviewed changes

cody-littley merged commit 841e0db into Layr-Labs:master Oct 15, 2024
6 checks passed

cody-littley deleted the table-store branch October 15, 2024 16:37


		var _ kvstore.Table = &tableView{}

		// tableView allows table in a New to be accessed as if it were a Store.

		@@ -0,0 +1,14 @@
		package kvstore

		// Batch is a collection of key / value pairs that can be written atomically to a database.

Table store #774

Table store #774

Conversation

cody-littley commented Sep 24, 2024 • edited Loading

Why are these changes needed?

Checks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianoaix Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cody-littley Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianoaix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cody-littley Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cody-littley commented Sep 24, 2024 •

edited

Loading

jianoaix Oct 7, 2024 •

edited

Loading

cody-littley Oct 9, 2024 •

edited

Loading

cody-littley Oct 10, 2024 •

edited

Loading