Add subject filters in schema relation delete to force use of the index #2131
Conversation
github-actions
bot
added
the
area/tooling
| @@ -11,64 +11,305 @@ import ( | |||
| "github.com/authzed/spicedb/pkg/datastore" | |||
| "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) { | |||
There was a problem hiding this comment.
we are missing a test with wildcards
internal/services/shared/schema.go
Outdated
|
|
||
| subjectSelectors := make([]datastore.SubjectsSelector, 0, len(previousRelation.TypeInformation.AllowedDirectRelations)) | ||
| for _, allowedType := range previousRelation.TypeInformation.AllowedDirectRelations { | ||
| if allowedType.GetRelation() != "" { |
There was a problem hiding this comment.
I think this worked by happenstance, but the ellipsis was not being handled correctly, as it's not modeled as empty string at this level of the stack, but ....
There is also no explicit handling of wildcards, but my understanding is that's modeled as an userset_object_id = *, but userset_relation = "", so it ended up falling in ellipsis branch, which would generate the wrong SubjectRelationFilter.
I've force-pushed the changes, let me know what you think
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.
vroldanbet
force-pushed
the
relation-remove-filters
branch
from
1d18f98
to
e8ad9bc
Compare
There was a problem hiding this comment.
I force pushed the changes, if you think they look fine, we can approve this.
We have some examples on how to test for Datastore API calls selecting specific indexes, I'd suggest adding a test to make sure this does not regress, but also confirm that what we wanted to achieve actually is happening.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
This should allow for much faster queries under the transaction