diff --git a/internal/datastore/benchmark/driver_bench_test.go b/internal/datastore/benchmark/driver_bench_test.go index f3b3534787..b4a4d65d32 100644 --- a/internal/datastore/benchmark/driver_bench_test.go +++ b/internal/datastore/benchmark/driver_bench_test.go @@ -54,6 +54,11 @@ var skipped = []string{ spanner.Engine, // Not useful to benchmark a simulator } +var sortOrders = map[string]options.SortOrder{ + "ByResource": options.ByResource, + "BySubject": options.BySubject, +} + func BenchmarkDatastoreDriver(b *testing.B) { for _, driver := range drivers { b.Run(driver.name+driver.suffix, func(b *testing.B) { @@ -114,49 +119,64 @@ func BenchmarkDatastoreDriver(b *testing.B) { } }) b.Run("SortedSnapshotReadOnlyNamespace", func(b *testing.B) { - for n := 0; n < b.N; n++ { - iter, err := ds.SnapshotReader(headRev).QueryRelationships(ctx, datastore.RelationshipsFilter{ - ResourceType: testfixtures.DocumentNS.Name, - }, options.WithSort(options.ByResource)) - require.NoError(b, err) - var count int - for rel := iter.Next(); rel != nil; rel = iter.Next() { - count++ - } - require.NoError(b, iter.Err()) - iter.Close() + for orderName, order := range sortOrders { + order := order + b.Run(orderName, func(b *testing.B) { + for n := 0; n < b.N; n++ { + iter, err := ds.SnapshotReader(headRev).QueryRelationships(ctx, datastore.RelationshipsFilter{ + ResourceType: testfixtures.DocumentNS.Name, + }, options.WithSort(order)) + require.NoError(b, err) + var count int + for rel := iter.Next(); rel != nil; rel = iter.Next() { + count++ + } + require.NoError(b, iter.Err()) + iter.Close() + } + }) } }) b.Run("SortedSnapshotReadWithRelation", func(b *testing.B) { - for n := 0; n < b.N; n++ { - iter, err := ds.SnapshotReader(headRev).QueryRelationships(ctx, datastore.RelationshipsFilter{ - ResourceType: testfixtures.DocumentNS.Name, - OptionalResourceRelation: "viewer", - }, options.WithSort(options.ByResource)) - require.NoError(b, err) - var count int - for rel := iter.Next(); rel != nil; rel = iter.Next() { - count++ - } - require.NoError(b, iter.Err()) - iter.Close() + for orderName, order := range sortOrders { + order := order + b.Run(orderName, func(b *testing.B) { + for n := 0; n < b.N; n++ { + iter, err := ds.SnapshotReader(headRev).QueryRelationships(ctx, datastore.RelationshipsFilter{ + ResourceType: testfixtures.DocumentNS.Name, + OptionalResourceRelation: "viewer", + }, options.WithSort(order)) + require.NoError(b, err) + var count int + for rel := iter.Next(); rel != nil; rel = iter.Next() { + count++ + } + require.NoError(b, iter.Err()) + iter.Close() + } + }) } }) b.Run("SortedSnapshotReadAllResourceFields", func(b *testing.B) { - for n := 0; n < b.N; n++ { - randDocNum := rand.Intn(numDocuments) - iter, err := ds.SnapshotReader(headRev).QueryRelationships(ctx, datastore.RelationshipsFilter{ - ResourceType: testfixtures.DocumentNS.Name, - OptionalResourceIds: []string{strconv.Itoa(randDocNum)}, - OptionalResourceRelation: "viewer", - }, options.WithSort(options.ByResource)) - require.NoError(b, err) - var count int - for rel := iter.Next(); rel != nil; rel = iter.Next() { - count++ - } - require.NoError(b, iter.Err()) - iter.Close() + for orderName, order := range sortOrders { + order := order + b.Run(orderName, func(b *testing.B) { + for n := 0; n < b.N; n++ { + randDocNum := rand.Intn(numDocuments) + iter, err := ds.SnapshotReader(headRev).QueryRelationships(ctx, datastore.RelationshipsFilter{ + ResourceType: testfixtures.DocumentNS.Name, + OptionalResourceIds: []string{strconv.Itoa(randDocNum)}, + OptionalResourceRelation: "viewer", + }, options.WithSort(order)) + require.NoError(b, err) + var count int + for rel := iter.Next(); rel != nil; rel = iter.Next() { + count++ + } + require.NoError(b, iter.Err()) + iter.Close() + } + }) } }) b.Run("SnapshotReverseRead", func(b *testing.B) { diff --git a/internal/datastore/common/sql.go b/internal/datastore/common/sql.go index 7ee6a63e30..8c89b7437c 100644 --- a/internal/datastore/common/sql.go +++ b/internal/datastore/common/sql.go @@ -127,86 +127,114 @@ func (sqf SchemaQueryFilterer) TupleOrder(order options.SortOrder) SchemaQueryFi sqf.schema.colUsersetObjectID, sqf.schema.colUsersetRelation, ) + + case options.BySubject: + sqf.queryBuilder = sqf.queryBuilder.OrderBy( + sqf.schema.colUsersetNamespace, + sqf.schema.colUsersetObjectID, + sqf.schema.colUsersetRelation, + sqf.schema.colNamespace, + sqf.schema.colObjectID, + sqf.schema.colRelation, + ) } return sqf } +type nameAndValue struct { + name string + value string +} + func (sqf SchemaQueryFilterer) After(cursor *core.RelationTuple, order options.SortOrder) SchemaQueryFilterer { - columnsAndValues := []struct { - name string - value string - }{ - { - sqf.schema.colNamespace, cursor.ResourceAndRelation.Namespace, - }, - { - sqf.schema.colObjectID, cursor.ResourceAndRelation.ObjectId, + columnsAndValues := map[options.SortOrder][]nameAndValue{ + options.ByResource: { + { + sqf.schema.colNamespace, cursor.ResourceAndRelation.Namespace, + }, + { + sqf.schema.colObjectID, cursor.ResourceAndRelation.ObjectId, + }, + { + sqf.schema.colRelation, cursor.ResourceAndRelation.Relation, + }, + { + sqf.schema.colUsersetNamespace, cursor.Subject.Namespace, + }, + { + sqf.schema.colUsersetObjectID, cursor.Subject.ObjectId, + }, + { + sqf.schema.colUsersetRelation, cursor.Subject.Relation, + }, }, - { - sqf.schema.colRelation, cursor.ResourceAndRelation.Relation, + options.BySubject: { + { + sqf.schema.colUsersetNamespace, cursor.Subject.Namespace, + }, + { + sqf.schema.colUsersetObjectID, cursor.Subject.ObjectId, + }, + { + sqf.schema.colUsersetRelation, cursor.Subject.Relation, + }, + { + sqf.schema.colNamespace, cursor.ResourceAndRelation.Namespace, + }, + { + sqf.schema.colObjectID, cursor.ResourceAndRelation.ObjectId, + }, + { + sqf.schema.colRelation, cursor.ResourceAndRelation.Relation, + }, }, - { - sqf.schema.colUsersetNamespace, cursor.Subject.Namespace, - }, - { - sqf.schema.colUsersetObjectID, cursor.Subject.ObjectId, - }, - { - sqf.schema.colUsersetRelation, cursor.Subject.Relation, - }, - } + }[order] switch sqf.schema.paginationFilterType { case TupleComparison: - switch order { - case options.ByResource: - // For performance reasons, remove any column names that have static values in the query. - columnNames := make([]string, 0, len(columnsAndValues)) - valueSlots := make([]any, 0, len(columnsAndValues)) - comparisonSlotCount := 0 - - for _, cav := range columnsAndValues { - if sqf.filteringColumnCounts[cav.name] != 1 { - columnNames = append(columnNames, cav.name) - valueSlots = append(valueSlots, cav.value) - comparisonSlotCount++ - } + // For performance reasons, remove any column names that have static values in the query. + columnNames := make([]string, 0, len(columnsAndValues)) + valueSlots := make([]any, 0, len(columnsAndValues)) + comparisonSlotCount := 0 + + for _, cav := range columnsAndValues { + if sqf.filteringColumnCounts[cav.name] != 1 { + columnNames = append(columnNames, cav.name) + valueSlots = append(valueSlots, cav.value) + comparisonSlotCount++ } + } - if comparisonSlotCount > 0 { - comparisonTuple := "(" + strings.Join(columnNames, ",") + ") > (" + strings.Repeat(",?", comparisonSlotCount)[1:] + ")" - sqf.queryBuilder = sqf.queryBuilder.Where( - comparisonTuple, - valueSlots..., - ) - } + if comparisonSlotCount > 0 { + comparisonTuple := "(" + strings.Join(columnNames, ",") + ") > (" + strings.Repeat(",?", comparisonSlotCount)[1:] + ")" + sqf.queryBuilder = sqf.queryBuilder.Where( + comparisonTuple, + valueSlots..., + ) } case ExpandedLogicComparison: - switch order { - case options.ByResource: - // For performance reasons, remove any column names that have static values in the query. - orClause := sq.Or{} - - for index, cav := range columnsAndValues { - if sqf.filteringColumnCounts[cav.name] != 1 { - andClause := sq.And{} - for _, previous := range columnsAndValues[0:index] { - if sqf.filteringColumnCounts[previous.name] != 1 { - andClause = append(andClause, sq.Eq{previous.name: previous.value}) - } + // For performance reasons, remove any column names that have static values in the query. + orClause := sq.Or{} + + for index, cav := range columnsAndValues { + if sqf.filteringColumnCounts[cav.name] != 1 { + andClause := sq.And{} + for _, previous := range columnsAndValues[0:index] { + if sqf.filteringColumnCounts[previous.name] != 1 { + andClause = append(andClause, sq.Eq{previous.name: previous.value}) } - - andClause = append(andClause, sq.Gt{cav.name: cav.value}) - orClause = append(orClause, andClause) } - } - if len(orClause) > 0 { - sqf.queryBuilder = sqf.queryBuilder.Where(orClause) + andClause = append(andClause, sq.Gt{cav.name: cav.value}) + orClause = append(orClause, andClause) } } + + if len(orClause) > 0 { + sqf.queryBuilder = sqf.queryBuilder.Where(orClause) + } } return sqf diff --git a/internal/datastore/memdb/readonly.go b/internal/datastore/memdb/readonly.go index 86a28c947b..12a0e6a11c 100644 --- a/internal/datastore/memdb/readonly.go +++ b/internal/datastore/memdb/readonly.go @@ -4,10 +4,14 @@ import ( "context" "fmt" "runtime" + "sort" + + "github.com/authzed/spicedb/pkg/spiceerrors" "github.com/hashicorp/go-memdb" "github.com/jzelinskie/stringz" + "github.com/authzed/spicedb/internal/datastore/common" "github.com/authzed/spicedb/pkg/datastore" "github.com/authzed/spicedb/pkg/datastore/options" core "github.com/authzed/spicedb/pkg/proto/core/v1" @@ -61,8 +65,20 @@ func (r *memdbReader) QueryRelationships( ) filteredIterator := memdb.NewFilterIterator(bestIterator, matchingRelationshipsFilterFunc) - iter := newMemdbTupleIterator(filteredIterator, queryOpts.Limit, queryOpts.Sort) - return iter, nil + switch queryOpts.Sort { + case options.Unsorted: + fallthrough + + case options.ByResource: + iter := newMemdbTupleIterator(filteredIterator, queryOpts.Limit, queryOpts.Sort) + return iter, nil + + case options.BySubject: + return newSubjectSortedIterator(filteredIterator, queryOpts.Limit) + + default: + return nil, spiceerrors.MustBugf("unsupported sort order: %v", queryOpts.Sort) + } } func mustHaveBeenClosed(iter *memdbTupleIterator) { @@ -350,11 +366,50 @@ func makeCursorFilterFn(after *core.RelationTuple, order options.SortOrder) func (less(tpl.subjectNamespace, tpl.subjectObjectID, tpl.subjectRelation, after.Subject) || eq(tpl.subjectNamespace, tpl.subjectObjectID, tpl.subjectRelation, after.Subject))) } + case options.BySubject: + return func(tpl *relationship) bool { + return less(tpl.subjectNamespace, tpl.subjectObjectID, tpl.subjectRelation, after.Subject) || + (eq(tpl.subjectNamespace, tpl.subjectObjectID, tpl.subjectRelation, after.Subject) && + (less(tpl.namespace, tpl.resourceID, tpl.relation, after.ResourceAndRelation) || + eq(tpl.subjectNamespace, tpl.subjectObjectID, tpl.subjectRelation, after.Subject))) + } } } return noopCursorFilter } +func newSubjectSortedIterator(it memdb.ResultIterator, limit *uint64) (datastore.RelationshipIterator, error) { + results := make([]*core.RelationTuple, 0) + + // Coalesce all of the results into memory + for foundRaw := it.Next(); foundRaw != nil; foundRaw = it.Next() { + rt, err := foundRaw.(*relationship).RelationTuple() + if err != nil { + return nil, err + } + + results = append(results, rt) + } + + // Sort them by subject + sort.Slice(results, func(i, j int) bool { + lhsRes := results[i].ResourceAndRelation + lhsSub := results[i].Subject + rhsRes := results[j].ResourceAndRelation + rhsSub := results[j].Subject + return less(lhsSub.Namespace, lhsSub.ObjectId, lhsSub.Relation, rhsSub) || + (eq(lhsSub.Namespace, lhsSub.ObjectId, lhsSub.Relation, rhsSub) && + (less(lhsRes.Namespace, lhsRes.ObjectId, lhsRes.Relation, rhsRes))) + }) + + // Limit them if requested + if limit != nil && uint64(len(results)) > *limit { + results = results[0:*limit] + } + + return common.NewSliceRelationshipIterator(results, options.BySubject), nil +} + func noopCursorFilter(_ *relationship) bool { return false } diff --git a/internal/testfixtures/datastore.go b/internal/testfixtures/datastore.go index 52b23e66de..a2e34d12d6 100644 --- a/internal/testfixtures/datastore.go +++ b/internal/testfixtures/datastore.go @@ -320,7 +320,7 @@ func (tc TupleChecker) VerifyOrderedIteratorResults(iter datastore.RelationshipI expectedStr := tuple.MustString(tpl) found := iter.Next() - tc.Require.NotNil(found, "expected %s", expectedStr) + tc.Require.NotNil(found, "expected %s, but found no additional results", expectedStr) foundStr := tuple.MustString(found) tc.Require.Equal(expectedStr, foundStr) diff --git a/pkg/datastore/options/options.go b/pkg/datastore/options/options.go index 5bca09b82f..d019eff04f 100644 --- a/pkg/datastore/options/options.go +++ b/pkg/datastore/options/options.go @@ -17,9 +17,10 @@ const ( // ByResource sorts the relationships by the resource component first ByResource - // NOTE: if you are intending to implement BySubject, it was fully implemented - // but not performance tested, and the code can be recovered from source control - // BySubject + // BySubject sorts the relationships by the subject component first. Note that + // BySubject might be quite a bit slower than ByResource, as relationships are + // indexed by resource. + BySubject ) type Cursor *core.RelationTuple diff --git a/pkg/datastore/test/pagination.go b/pkg/datastore/test/pagination.go index ad644f03b6..53fff2ac94 100644 --- a/pkg/datastore/test/pagination.go +++ b/pkg/datastore/test/pagination.go @@ -18,12 +18,16 @@ import ( func OrderingTest(t *testing.T, tester DatastoreTester) { testCases := []struct { - objectType string - ordering options.SortOrder + resourceType string + ordering options.SortOrder }{ {testfixtures.DocumentNS.Name, options.ByResource}, {testfixtures.FolderNS.Name, options.ByResource}, {testfixtures.UserNS.Name, options.ByResource}, + + {testfixtures.DocumentNS.Name, options.BySubject}, + {testfixtures.FolderNS.Name, options.BySubject}, + {testfixtures.UserNS.Name, options.BySubject}, } rawDS, err := tester.New(0, veryLargeGCInterval, veryLargeGCWindow, 1) @@ -33,15 +37,17 @@ func OrderingTest(t *testing.T, tester DatastoreTester) { tRequire := testfixtures.TupleChecker{Require: require.New(t), DS: ds} for _, tc := range testCases { - t.Run(fmt.Sprintf("%s-%d", tc.objectType, tc.ordering), func(t *testing.T) { + tc := tc + + t.Run(fmt.Sprintf("%s-%d", tc.resourceType, tc.ordering), func(t *testing.T) { require := require.New(t) ctx := context.Background() - expected := sortedStandardData(tc.objectType, tc.ordering) + expected := sortedStandardData(tc.resourceType, tc.ordering) // Check the snapshot reader order iter, err := ds.SnapshotReader(rev).QueryRelationships(ctx, datastore.RelationshipsFilter{ - ResourceType: tc.objectType, + ResourceType: tc.resourceType, }, options.WithSort(tc.ordering)) require.NoError(err) @@ -65,7 +71,7 @@ func OrderingTest(t *testing.T, tester DatastoreTester) { // Check a reader from with a transaction _, err = ds.ReadWriteTx(ctx, func(rwt datastore.ReadWriteTransaction) error { iter, err := rwt.QueryRelationships(ctx, datastore.RelationshipsFilter{ - ResourceType: tc.objectType, + ResourceType: tc.resourceType, }, options.WithSort(tc.ordering)) require.NoError(err) defer iter.Close() @@ -384,13 +390,13 @@ func foreachTxType( }) } -func sortedStandardData(objectType string, order options.SortOrder) []*core.RelationTuple { +func sortedStandardData(resourceType string, order options.SortOrder) []*core.RelationTuple { asTuples := lo.Map(testfixtures.StandardTuples, func(item string, _ int) *core.RelationTuple { return tuple.Parse(item) }) filteredToType := lo.Filter(asTuples, func(item *core.RelationTuple, _ int) bool { - return item.ResourceAndRelation.Namespace == objectType + return item.ResourceAndRelation.Namespace == resourceType }) sort.Slice(filteredToType, func(i, j int) bool { @@ -401,6 +407,8 @@ func sortedStandardData(objectType string, order options.SortOrder) []*core.Rela switch order { case options.ByResource: return lhsResource < rhsResource || (lhsResource == rhsResource && lhsSubject < rhsSubject) + case options.BySubject: + return lhsSubject < rhsSubject || (lhsSubject == rhsSubject && lhsResource < rhsResource) default: panic("request for sorted test data with no sort order") }