Merge pull request #839 from josephschorr/direct-check-performance-im…

…provements

Direct check performance improvements

.github/workflows/build-test.yaml

@@ -93,7 +93,7 @@ jobs:
       - uses: "authzed/actions/go-test@main"
         with:
           tags: "ci,docker"
-          timeout: "10m"
+          timeout: "15m"
           working_directory: "internal/services/integrationtesting"
 
   datastore:
@@ -114,6 +114,24 @@ jobs:
           tags: "ci,docker"
           timeout: "10m"
           working_directory: "internal/datastore/${{ matrix.datastore }}"
+
+  datastoreconsistency:
+    name: "Datastore Consistency Tests"
+    runs-on: "ubuntu-latest"
+    strategy:
+      fail-fast: false
+      matrix:
+        datastore: ["cockroachdb", "mysql", "postgres", "spanner"]
+    steps:
+      - uses: "actions/checkout@v3"
+      - uses: "actions/setup-go@v3"
+        with:
+          go-version: "${{ env.GO_VERSION }}"
+          cache: "true"
+      - name: "Run Datastore Consistency Tests"
+        working-directory: "internal/services/integrationtesting"
+        run: "go test --failfast -count=1 -timeout '10m' --tags='ci,docker,datastoreconsistency' ./... -run TestConsistencyPerDatastore/${{ matrix.datastore }}"
+
   e2e:
     name: "E2E"
     runs-on: "ubuntu-latest"

e2e/go.mod

@@ -34,6 +34,7 @@ require (
 	github.com/jackc/pgpassfile v1.0.0 // indirect
 	github.com/jackc/pgproto3/v2 v2.3.1 // indirect
 	github.com/jackc/pgservicefile v0.0.0-20200714003250-2b9c44734f2b // indirect
+	github.com/jzelinskie/stringz v0.0.1 // indirect
 	github.com/mattn/go-colorable v0.1.12 // indirect
 	github.com/mattn/go-isatty v0.0.16 // indirect
 	github.com/pmezard/go-difflib v1.0.0 // indirect

e2e/go.sum

@@ -146,6 +146,8 @@ github.com/jackc/puddle v0.0.0-20190413234325-e4ced69a3a2b/go.mod h1:m4B5Dj62Y0f
 github.com/jackc/puddle v0.0.0-20190608224051-11cab39313c9/go.mod h1:m4B5Dj62Y0fbyuIc15OsIqK0+JU8nkqQjsgx7dvjSWk=
 github.com/jackc/puddle v1.1.3/go.mod h1:m4B5Dj62Y0fbyuIc15OsIqK0+JU8nkqQjsgx7dvjSWk=
 github.com/jackc/puddle v1.3.0/go.mod h1:m4B5Dj62Y0fbyuIc15OsIqK0+JU8nkqQjsgx7dvjSWk=
+github.com/jzelinskie/stringz v0.0.1 h1:IahR+y8ct2nyj7B6i8UtFsGFj4ex1SX27iKFYsAheLk=
+github.com/jzelinskie/stringz v0.0.1/go.mod h1:hHYbgxJuNLRw91CmpuFsYEOyQqpDVFg8pvEh23vy4P0=
 github.com/kisielk/errcheck v1.5.0/go.mod h1:pFxgyoBC7bSaBwPgfKdkLd5X25qrDl4LWUI2bnpBCr8=
 github.com/kisielk/gotool v1.0.0/go.mod h1:XhKaO+MFFWcvkIS/tQcRk01m1F5IRFswLeQ+oQHNcck=
 github.com/konsorten/go-windows-terminal-sequences v1.0.1/go.mod h1:T0+1ngSBFLxvqU3pZ+m/2kptfBszLMUkC4ZK/EgS/cQ=

internal/datastore/common/sql.go

@@ -6,8 +6,6 @@ import (
 	"math"
 	"runtime"
 
-	"github.com/authzed/spicedb/pkg/spiceerrors"
-
 	sq "github.com/Masterminds/squirrel"
 	v1 "github.com/authzed/authzed-go/proto/authzed/api/v1"
 	"github.com/jzelinskie/stringz"
@@ -17,6 +15,7 @@ import (
 	"github.com/authzed/spicedb/internal/datastore/options"
 	"github.com/authzed/spicedb/pkg/datastore"
 	core "github.com/authzed/spicedb/pkg/proto/core/v1"
+	"github.com/authzed/spicedb/pkg/spiceerrors"
 )
 
 var (
@@ -166,8 +165,8 @@ func (sqf SchemaQueryFilterer) FilterWithRelationshipsFilter(filter datastore.Re
 		sqf = usqf
 	}
 
-	if filter.OptionalSubjectsFilter != nil {
-		usqf, err := sqf.FilterWithSubjectsFilter(*filter.OptionalSubjectsFilter)
+	if len(filter.OptionalSubjectsSelectors) > 0 {
+		usqf, err := sqf.FilterWithSubjectsSelectors(filter.OptionalSubjectsSelectors...)
 		if err != nil {
 			return sqf, err
 		}
@@ -181,75 +180,90 @@ func (sqf SchemaQueryFilterer) FilterWithRelationshipsFilter(filter datastore.Re
 	return sqf, nil
 }
 
-// MustFilterWithSubjectsFilter returns a new SchemaQueryFilterer that is limited to resources with
-// subjects that match the specified filter.
-func (sqf SchemaQueryFilterer) MustFilterWithSubjectsFilter(filter datastore.SubjectsFilter) SchemaQueryFilterer {
-	usqf, err := sqf.FilterWithSubjectsFilter(filter)
+// MustFilterWithSubjectsSelectors returns a new SchemaQueryFilterer that is limited to resources with
+// subjects that match the specified selector(s).
+func (sqf SchemaQueryFilterer) MustFilterWithSubjectsSelectors(selectors ...datastore.SubjectsSelector) SchemaQueryFilterer {
+	usqf, err := sqf.FilterWithSubjectsSelectors(selectors...)
 	if err != nil {
 		panic(err)
 	}
 	return usqf
 }
 
-// FilterWithSubjectsFilter returns a new SchemaQueryFilterer that is limited to resources with
-// subjects that match the specified filter.
-func (sqf SchemaQueryFilterer) FilterWithSubjectsFilter(filter datastore.SubjectsFilter) (SchemaQueryFilterer, error) {
-	sqf.queryBuilder = sqf.queryBuilder.Where(sq.Eq{sqf.schema.ColUsersetNamespace: filter.SubjectType})
-	sqf.tracerAttributes = append(sqf.tracerAttributes, SubNamespaceNameKey.String(filter.SubjectType))
+// FilterWithSubjectsSelectors returns a new SchemaQueryFilterer that is limited to resources with
+// subjects that match the specified selector(s).
+func (sqf SchemaQueryFilterer) FilterWithSubjectsSelectors(selectors ...datastore.SubjectsSelector) (SchemaQueryFilterer, error) {
+	selectorsOrClause := sq.Or{}
 
-	if len(filter.OptionalSubjectIds) > 0 {
-		// TODO(jschorr): Change this panic into an automatic query split, if we find it necessary.
-		if len(filter.OptionalSubjectIds) > datastore.FilterMaximumIDCount {
-			return sqf, spiceerrors.MustBugf("cannot have more than %d subject IDs in a single filter", datastore.FilterMaximumIDCount)
-		}
+	for _, selector := range selectors {
+		selectorClause := sq.And{}
 
-		inClause := fmt.Sprintf("%s IN (", sqf.schema.ColUsersetObjectID)
-		args := make([]any, 0, len(filter.OptionalSubjectIds))
+		if len(selector.OptionalSubjectType) > 0 {
+			selectorClause = append(selectorClause, sq.Eq{sqf.schema.ColUsersetNamespace: selector.OptionalSubjectType})
+			sqf.tracerAttributes = append(sqf.tracerAttributes, SubNamespaceNameKey.String(selector.OptionalSubjectType))
+		}
 
-		for index, subjectID := range filter.OptionalSubjectIds {
-			if len(subjectID) == 0 {
-				return sqf, spiceerrors.MustBugf("got empty subject ID")
+		if len(selector.OptionalSubjectIds) > 0 {
+			// TODO(jschorr): Change this panic into an automatic query split, if we find it necessary.
+			if len(selector.OptionalSubjectIds) > datastore.FilterMaximumIDCount {
+				return sqf, spiceerrors.MustBugf("cannot have more than %d subject IDs in a single filter", datastore.FilterMaximumIDCount)
 			}
 
-			if index > 0 {
-				inClause += ", "
-			}
+			inClause := fmt.Sprintf("%s IN (", sqf.schema.ColUsersetObjectID)
+			args := make([]any, 0, len(selector.OptionalSubjectIds))
 
-			inClause += "?"
+			for index, subjectID := range selector.OptionalSubjectIds {
+				if len(subjectID) == 0 {
+					return sqf, spiceerrors.MustBugf("got empty subject ID")
+				}
 
-			args = append(args, subjectID)
-			sqf.tracerAttributes = append(sqf.tracerAttributes, SubObjectIDKey.String(subjectID))
-		}
+				if index > 0 {
+					inClause += ", "
+				}
 
-		sqf.queryBuilder = sqf.queryBuilder.Where(inClause+")", args...)
-	}
+				inClause += "?"
 
-	if !filter.RelationFilter.IsEmpty() {
-		relations := make([]string, 0, 2)
-		if filter.RelationFilter.IncludeEllipsisRelation {
-			relations = append(relations, datastore.Ellipsis)
-		}
+				args = append(args, subjectID)
+				sqf.tracerAttributes = append(sqf.tracerAttributes, SubObjectIDKey.String(subjectID))
+			}
 
-		if filter.RelationFilter.NonEllipsisRelation != "" {
-			relations = append(relations, filter.RelationFilter.NonEllipsisRelation)
+			selectorClause = append(selectorClause, sq.Expr(inClause+")", args...))
 		}
 
-		if len(relations) == 1 {
-			relName := relations[0]
-			sqf.tracerAttributes = append(sqf.tracerAttributes, SubRelationNameKey.String(relName))
-			sqf.queryBuilder = sqf.queryBuilder.Where(sq.Eq{sqf.schema.ColUsersetRelation: relName})
-		} else {
-			orClause := sq.Or{}
-			for _, relationName := range relations {
-				dsRelationName := stringz.DefaultEmpty(relationName, datastore.Ellipsis)
-				orClause = append(orClause, sq.Eq{sqf.schema.ColUsersetRelation: dsRelationName})
-				sqf.tracerAttributes = append(sqf.tracerAttributes, SubRelationNameKey.String(dsRelationName))
+		if !selector.RelationFilter.IsEmpty() {
+			if selector.RelationFilter.OnlyNonEllipsisRelations {
+				selectorClause = append(selectorClause, sq.NotEq{sqf.schema.ColUsersetRelation: datastore.Ellipsis})
+			} else {
+				relations := make([]string, 0, 2)
+				if selector.RelationFilter.IncludeEllipsisRelation {
+					relations = append(relations, datastore.Ellipsis)
+				}
+
+				if selector.RelationFilter.NonEllipsisRelation != "" {
+					relations = append(relations, selector.RelationFilter.NonEllipsisRelation)
+				}
+
+				if len(relations) == 1 {
+					relName := relations[0]
+					sqf.tracerAttributes = append(sqf.tracerAttributes, SubRelationNameKey.String(relName))
+					selectorClause = append(selectorClause, sq.Eq{sqf.schema.ColUsersetRelation: relName})
+				} else {
+					orClause := sq.Or{}
+					for _, relationName := range relations {
+						dsRelationName := stringz.DefaultEmpty(relationName, datastore.Ellipsis)
+						orClause = append(orClause, sq.Eq{sqf.schema.ColUsersetRelation: dsRelationName})
+						sqf.tracerAttributes = append(sqf.tracerAttributes, SubRelationNameKey.String(dsRelationName))
+					}
+
+					selectorClause = append(selectorClause, orClause)
+				}
 			}
-
-			sqf.queryBuilder = sqf.queryBuilder.Where(orClause)
 		}
+
+		selectorsOrClause = append(selectorsOrClause, selectorClause)
 	}
 
+	sqf.queryBuilder = sqf.queryBuilder.Where(selectorsOrClause)
 	return sqf, nil
 }