From 132ed3b6ec98c4a00a10243b99d92636c2be3005 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Tue, 12 Nov 2024 17:23:22 -0500 Subject: [PATCH 1/2] Add subject filters in schema relation delete to force use of the index This should allow for much faster queries under the transaction. We observed some customers reporting timeouts when writting a new schema and dropping a relation because it ended up in full table scans. --- internal/services/shared/schema.go | 42 ++- internal/services/shared/schema_test.go | 367 ++++++++++++++++++++---- 2 files changed, 351 insertions(+), 58 deletions(-) diff --git a/internal/services/shared/schema.go b/internal/services/shared/schema.go index 7263ddcb57..219885bc6a 100644 --- a/internal/services/shared/schema.go +++ b/internal/services/shared/schema.go @@ -319,10 +319,46 @@ func sanityCheckNamespaceChanges( for _, delta := range diff.Deltas() { switch delta.Type { case nsdiff.RemovedRelation: + // NOTE: We add the subject filters here to ensure the reverse relationship index is used + // by the datastores. As there is no index that has {namespace, relation} directly, but there + // *is* an index that has {subject_namespace, subject_relation, namespace, relation}, we can + // force the datastore to use the reverse index by adding the subject filters. + var previousRelation *core.Relation + for _, relation := range existing.Relation { + if relation.Name == delta.RelationName { + previousRelation = relation + break + } + } + + if previousRelation == nil { + return nil, spiceerrors.MustBugf("relation `%s` not found in existing namespace definition", delta.RelationName) + } + + subjectSelectors := make([]datastore.SubjectsSelector, 0, len(previousRelation.TypeInformation.AllowedDirectRelations)) + for _, allowedType := range previousRelation.TypeInformation.AllowedDirectRelations { + if allowedType.GetRelation() == datastore.Ellipsis { + subjectSelectors = append(subjectSelectors, datastore.SubjectsSelector{ + OptionalSubjectType: allowedType.Namespace, + RelationFilter: datastore.SubjectRelationFilter{ + IncludeEllipsisRelation: true, + }, + }) + } else { + subjectSelectors = append(subjectSelectors, datastore.SubjectsSelector{ + OptionalSubjectType: allowedType.Namespace, + RelationFilter: datastore.SubjectRelationFilter{ + NonEllipsisRelation: allowedType.GetRelation(), + }, + }) + } + } + qy, qyErr := rwt.QueryRelationships(ctx, datastore.RelationshipsFilter{ - OptionalResourceType: nsdef.Name, - OptionalResourceRelation: delta.RelationName, - }) + OptionalResourceType: nsdef.Name, + OptionalResourceRelation: delta.RelationName, + OptionalSubjectsSelectors: subjectSelectors, + }, options.WithLimit(options.LimitOne)) err = errorIfTupleIteratorReturnsTuples( ctx, diff --git a/internal/services/shared/schema_test.go b/internal/services/shared/schema_test.go index 78012466f4..007af287db 100644 --- a/internal/services/shared/schema_test.go +++ b/internal/services/shared/schema_test.go @@ -9,66 +9,323 @@ import ( "github.com/authzed/spicedb/internal/datastore/memdb" "github.com/authzed/spicedb/internal/testfixtures" "github.com/authzed/spicedb/pkg/datastore" + corev1 "github.com/authzed/spicedb/pkg/proto/core/v1" "github.com/authzed/spicedb/pkg/schemadsl/compiler" "github.com/authzed/spicedb/pkg/schemadsl/input" + "github.com/authzed/spicedb/pkg/tuple" ) func TestApplySchemaChanges(t *testing.T) { - require := require.New(t) - rawDS, err := memdb.NewMemdbDatastore(0, 0, memdb.DisableGC) - require.NoError(err) - - // Write the initial schema. - ds, _ := testfixtures.DatastoreFromSchemaAndTestRelationships(rawDS, ` - definition user {} - - definition document { - relation viewer: user - permission view = viewer - } - - caveat hasFortyTwo(value int) { - value == 42 - } - `, nil, require) - - // Update the schema and ensure it works. - compiled, err := compiler.Compile(compiler.InputSchema{ - Source: input.Source("schema"), - SchemaString: ` - definition user {} - - definition organization { - relation member: user - permission admin = member - } + tcs := []struct { + name string + startingSchema string + relationships []string + endingSchema string + expectedAppliedSchemaChanges AppliedSchemaChanges + expectedError string + }{ + { + name: "various changes", + startingSchema: ` + definition user {} + + definition document { + relation viewer: user + permission view = viewer + } + + caveat hasFortyTwo(value int) { + value == 42 + } + `, + endingSchema: ` + definition user {} + + definition organization { + relation member: user + permission admin = member + } + + caveat catchTwentyTwo(value int) { + value == 22 + } + `, + expectedAppliedSchemaChanges: AppliedSchemaChanges{ + TotalOperationCount: 5, + NewObjectDefNames: []string{"organization"}, + RemovedObjectDefNames: []string{"document"}, + NewCaveatDefNames: []string{"catchTwentyTwo"}, + RemovedCaveatDefNames: []string{"hasFortyTwo"}, + }, + }, + { + name: "attempt to remove a relation with relationships", + startingSchema: ` + definition user {} + + definition group { + relation member: user + } + + definition org { + relation admin: user + } + + definition document { + relation viewer: user | group#member | org#admin + }`, + relationships: []string{"document:somedoc#viewer@user:alice"}, + endingSchema: ` + definition user {} + + definition group { + relation member: user + } + + definition org { + relation admin: user + } + + definition document {}`, + expectedError: "cannot delete relation `viewer` in object definition `document`, as a relationship exists under it", + }, + { + name: "attempt to remove a relation with indirect relationships", + startingSchema: ` + definition user {} + + definition group { + relation member: user + } + + definition org { + relation admin: user + } + + definition document { + relation viewer: user | group#member | org#admin + }`, + relationships: []string{"document:somedoc#viewer@group:somegroup#member"}, + endingSchema: ` + definition user {} + + definition group { + relation member: user + } + + definition org { + relation admin: user + } + + definition document {}`, + expectedError: "cannot delete relation `viewer` in object definition `document`, as a relationship exists under it", + }, + { + name: "attempt to remove a relation with other indirect relationships", + startingSchema: ` + definition user {} + + definition group { + relation member: user + } + + definition org { + relation admin: user + } + + definition document { + relation viewer: user | group#member | org#admin + }`, + relationships: []string{"document:somedoc#viewer@org:someorg#admin"}, + endingSchema: ` + definition user {} + + definition group { + relation member: user + } + + definition org { + relation admin: user + } + + definition document {}`, + expectedError: "cannot delete relation `viewer` in object definition `document`, as a relationship exists under it", + }, + { + name: "attempt to remove a relation with wildcard", + startingSchema: ` + definition user {} + + definition document { + relation viewer: user:* | user + }`, + relationships: []string{"document:somedoc#viewer@user:*"}, + endingSchema: ` + definition user {} + + definition document {}`, + expectedError: "cannot delete relation `viewer` in object definition `document`, as a relationship exists under it", + }, + { + name: "attempt to remove a relation with only indirect relationships", + startingSchema: ` + definition user {} + + definition group { + relation member: user + } - caveat catchTwentyTwo(value int) { - value == 22 + definition org { + relation admin: user + } + + definition document { + relation viewer: group#member | org#admin + }`, + relationships: []string{"document:somedoc#viewer@org:someorg#admin"}, + endingSchema: ` + definition user {} + + definition group { + relation member: user + } + + definition org { + relation admin: user + } + + definition document {}`, + expectedError: "cannot delete relation `viewer` in object definition `document`, as a relationship exists under it", + }, + { + name: "remove a relation with no relationships", + startingSchema: ` + definition user {} + + definition group { + relation member: user + } + + definition org { + relation admin: user + } + + definition document { + relation viewer: user | group#member | org#admin + }`, + endingSchema: ` + definition user {} + + definition group { + relation member: user + } + + definition org { + relation admin: user + } + + definition document {}`, + expectedAppliedSchemaChanges: AppliedSchemaChanges{ + TotalOperationCount: 4, + }, + }, + { + name: "change the subject type allowed on a relation", + startingSchema: ` + definition user {} + + definition group { + relation member: user + } + + definition document { + relation viewer: user | group#member + permission view = viewer + } + `, + endingSchema: ` + definition user {} + + definition group { + relation member: user + } + + definition document { + relation viewer: user + permission view = viewer + } + `, + expectedAppliedSchemaChanges: AppliedSchemaChanges{ + TotalOperationCount: 3, + }, + }, + { + name: "attempt to change the subject type allowed on a relation with relationships", + startingSchema: ` + definition user {} + + definition group { + relation member: user + } + + definition document { + relation viewer: user | group#member + permission view = viewer + } + `, + relationships: []string{"document:somedoc#viewer@group:somegroup#member"}, + endingSchema: ` + definition user {} + + definition group { + relation member: user + } + + definition document { + relation viewer: user + permission view = viewer + } + `, + expectedError: "cannot remove allowed type `group#member` from relation `viewer` in object definition `document`, as a relationship exists with it", + }, + } + + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + require := require.New(t) + rawDS, err := memdb.NewMemdbDatastore(0, 0, memdb.DisableGC) + require.NoError(err) + + // Write the initial schema. + relationships := make([]*corev1.RelationTuple, 0, len(tc.relationships)) + for _, rel := range tc.relationships { + relationships = append(relationships, tuple.MustParse(rel)) } - `, - }, compiler.AllowUnprefixedObjectType()) - require.NoError(err) - - validated, err := ValidateSchemaChanges(context.Background(), compiled, false) - require.NoError(err) - - _, err = ds.ReadWriteTx(context.Background(), func(ctx context.Context, rwt datastore.ReadWriteTransaction) error { - applied, err := ApplySchemaChanges(context.Background(), rwt, validated) - require.NoError(err) - - require.Equal(applied.NewObjectDefNames, []string{"organization"}) - require.Equal(applied.RemovedObjectDefNames, []string{"document"}) - require.Equal(applied.NewCaveatDefNames, []string{"catchTwentyTwo"}) - require.Equal(applied.RemovedCaveatDefNames, []string{"hasFortyTwo"}) - - orgDef, err := rwt.LookupNamespacesWithNames(ctx, []string{"organization"}) - require.NoError(err) - require.Len(orgDef, 1) - - require.NotEmpty(orgDef[0].Definition.Relation[0].CanonicalCacheKey) - require.NotEmpty(orgDef[0].Definition.Relation[1].CanonicalCacheKey) - return nil - }) - require.NoError(err) + + ds, _ := testfixtures.DatastoreFromSchemaAndTestRelationships(rawDS, tc.startingSchema, relationships, require) + + // Update the schema and ensure it works. + compiled, err := compiler.Compile(compiler.InputSchema{ + Source: input.Source("schema"), + SchemaString: tc.endingSchema, + }, compiler.AllowUnprefixedObjectType()) + require.NoError(err) + + validated, err := ValidateSchemaChanges(context.Background(), compiled, false) + require.NoError(err) + + _, err = ds.ReadWriteTx(context.Background(), func(ctx context.Context, rwt datastore.ReadWriteTransaction) error { + applied, err := ApplySchemaChanges(context.Background(), rwt, validated) + if tc.expectedError != "" { + require.EqualError(err, tc.expectedError) + return nil + } + + require.NoError(err) + require.Equal(tc.expectedAppliedSchemaChanges, *applied) + return nil + }) + require.NoError(err) + }) + } } From cb515f0b4167618448491de26b27e667a240e470 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Mon, 4 Nov 2024 18:10:06 -0500 Subject: [PATCH 2/2] Fix MySQL test breakage caused by daylight savings change Seriously, it broke due to the DST change because it just *happen* to match the -5 hours window used for the test. The fix is to move the test that changes the instance's timezone to its own, isolated, MySQL so it doesn't interfere with the other tests --- internal/datastore/mysql/datastore_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/internal/datastore/mysql/datastore_test.go b/internal/datastore/mysql/datastore_test.go index 671749f595..fb197e2e58 100644 --- a/internal/datastore/mysql/datastore_test.go +++ b/internal/datastore/mysql/datastore_test.go @@ -106,6 +106,11 @@ func TestMySQL8Datastore(t *testing.T) { additionalMySQLTests(t, b) } +func TestMySQLRevisionTimestamps(t *testing.T) { + b := testdatastore.RunMySQLForTestingWithOptions(t, testdatastore.MySQLTesterOptions{MigrateForNewDatastore: true}, "") + t.Run("TransactionTimestamps", createDatastoreTest(b, TransactionTimestampsTest, defaultOptions...)) +} + func additionalMySQLTests(t *testing.T, b testdatastore.RunningEngineForTest) { reg := prometheus.NewRegistry() prometheus.DefaultGatherer = reg @@ -122,7 +127,6 @@ func additionalMySQLTests(t *testing.T, b testdatastore.RunningEngineForTest) { t.Run("ChunkedGarbageCollection", createDatastoreTest(b, ChunkedGarbageCollectionTest, defaultOptions...)) t.Run("EmptyGarbageCollection", createDatastoreTest(b, EmptyGarbageCollectionTest, defaultOptions...)) t.Run("NoRelationshipsGarbageCollection", createDatastoreTest(b, NoRelationshipsGarbageCollectionTest, defaultOptions...)) - t.Run("TransactionTimestamps", createDatastoreTest(b, TransactionTimestampsTest, defaultOptions...)) t.Run("QuantizedRevisions", func(t *testing.T) { QuantizedRevisionTest(t, b) })