spicedb icon indicating copy to clipboard operation
spicedb copied to clipboard

Update (bulk delete and touch) relationships atomically

Open henkosch opened this issue 3 years ago • 7 comments
trafficstars

I have an example schema like this:

definition user {
    relation isin: domain
}

definition domain {
}

In this schema a user is only allowed to be in a SINGLE domain at a time! Let's say I have the following relationship existing in the database:

user:u1#isin@domain:d1

Then my application generates an event, that u1 now should belong to d2 so I want to update my spicedb relations accordingly in an atomic way.

This could be done simply, by calling: (pseudocode for grpc)

PermissionService.WriteRelationships({
    operations: [
        { operation: DELETE, relationship: "user:u1#isin@domain:d1"},
        { operation: TOUCH, relationship: "user:u1#isin@domain:d2"},
    ]
})

Of course only if you know the previous domain id when you are creating the grpc request for the delete. But my application cannot be absolutely sure that spicedb is fully in sync with it's previous state, so the above can result in an error if for some reason "user:u1#isin@domain:d1" was not there in spicedb. Maybe some other subsystem modified it in spicedb and the caller did not know about it.

My intention is that I want to set spicedb to the new state whatever the old state was in it. So I have 2 options:

  1. read the previous relationships first with ReadRelationships and then generate the DELETE + TOUCH operations using the result of the READ as the input of the DELETE(s), but the READ operation could be slow and it would not be atomic anymore
  2. BULK_DELETE with a RelationshipFilter "user:u1#isin@domain:*" so it deletes all isin relations for this specific user then perform just a TOUCH for the new state
PermissionService.DeleteRelationships({
    relationshipFilter: "user:u1#isin@domain:*"
})
PermissionService.WriteRelationships({
    operations: [
        { operation: TOUCH, relationship: "user:u1#isin@domain:d2"},
    ]
})

Is there a way to put a RelationshipFilter expression into the WriteRelationships somehow so it could be atomic? Is this maybe planned?

Something like this maybe?

PermissionService.WriteRelationships({
    operations: [
        { operation: BULK_DELETE, relationshipFilter: "user:u1#isin@domain:*" },
        { operation: TOUCH, relationship: "user:u1#isin@domain:d2"},
    ]
})

What do you think?

henkosch avatar Oct 07 '22 11:10 henkosch

@henkosch 👋🏻 thanks for bringing this to our attention. What datastore are you using? A few options occur to me:

  • make DELETE in the WriteRelationships idempotent, so that if the relationship does not exist it does not fail
  • have you application do a DELETE+TOUCH using one or more Precondition that makes sure the domain exists (or that it does not, if that was determined in a previous non-atomic read). If it does not, application would need to retry operation, doing a READ again and adjusting the WriteRelationships request.

Enabling a bulk delete operation sounds like a potential interesting addition. We already support that in DeleteRelationships RPC method. We could explore adding a RelationshipFilter to RelationshipUpdate, or a new type of RelationshipUpdate_Operation. @josephschorr thoughts? I guess if we implemented such a thing, there wouldn't need to mantain DeleteRelationships RPC anymore?

vroldanbet avatar Oct 07 '22 11:10 vroldanbet

@vroldanbet Thank you! We're using PostgreSQL, so as I saw it in the source code it uses the Serializable transaction isolation level inside the WriteRelationships RPC service. As for the first two options, you have mentioned, those would only solve the problem partially, so that the new relationship would be surely written to the database whatever the previous value was, but it would not clean up any remaining unknown old relationships from there for the same user, to keep the constraint that a specific user should have only one isin relation at a time to a domain.

An other option could be to create BeginTransaction / Commit / Rollback RPC calls to manage transaction boundaries from the caller application, but I doubt that it would be worth keeping a db connection alive across multiple RPC calls, not to mention a lot of challenges it would bring to the architecture.

Adding the RelationshipFilter to the RelationshipUpdate RPC sounds really good to me! That would allow all sorts of data modification operations to run in a single database transaction without introducing too much complexity. Do you see any challenges with it?

henkosch avatar Oct 07 '22 12:10 henkosch

those would only solve the problem partially, so that the new relationship would be surely written to the database whatever the previous value was, but it would not clean up any remaining unknown old relationships from there for the same user, to keep the constraint that a specific user should have only one isin relation at a time to a domain.

right, my assumption was application code would be responsible to identify the domain the user belongs to, and then turn that into a Precondition. Because all of this happens transactionally, if between the app reads the state and the SpiceDB tx is executed anything changed the Preconditions would catch it:

1. Make sure domain A exists
2. Delete Domain A
3. Create Domain B

If any other competing transaction tries to do the same, it would have to also attempt to delete the current domain, and it would case other transactions to fail and make the client retry. If client app does not keep track of domains, then it gets tricky, you are right.

Idempotent deletes would indeed not work here, as you cannot prevent any other competing transaction from deleting the domain first, leaving the second transaction attempting to delete a transaction no longer exists.

An other option could be to create BeginTransaction / Commit / Rollback RPC calls to manage transaction boundaries from the caller application, but I doubt that it would be worth keeping a db connection alive across multiple RPC calls, not to mention a lot of challenges it would bring to the architecture.

We've been exploring this option as a solution to the "dual-write problem" by implementing X/OpenXA spec or our own 2PC API. This could help you indeed, although I don't think we can pull this off any time soon in a small PR 😄

Adding the RelationshipFilter to the RelationshipUpdate RPC sounds really good to me! That would allow all sorts of data modification operations to run in a single database transaction without introducing too much complexity. Do you see any challenges with it?

I don't see any challenges but I'm new to the codebase so I may be overlooking something 😅 I like the idea because I feel the most common case would be transactional operations that combine writes and deletes, and it would allow us to use a single API method. Let's wait for other folks from the authzed team to chime in

vroldanbet avatar Oct 07 '22 13:10 vroldanbet

Thanks! Well, right now we don't have the previous domain id in our entity update event, that is why I started looking into options how to delete all the previous relations for the old domains. It's a good idea to check it with a precondition, but we have to have the old values in the update event on our messaging system, to write the precondition.

For adding the bulk deletes to the RelationshipUpdate I was looking at the proto to figure out what would be the least disrupting way to add that, but I think it gets a little bit tricky to keep it backward compatible.

Here are my ideas:

Option 1:

enum Operation {
	OPERATION_UNSPECIFIED = 0;
	OPERATION_CREATE = 1;
	OPERATION_TOUCH = 2;
	OPERATION_DELETE = 3;
        OPERATION_DELETE_BULK = 4; // new
}

message RelationshipUpdate {
	RelationshipUpdate.Operation operation = 1;
	Relationship relationship = 2; // this might need to be changed to optional, too :(
	RelationshipFilter optional_relationship_filter = 3; // new
}

Option 2:

message WriteRelationshipsRequest {
	repeated RelationshipUpdate updates = 1;
	repeated Precondition optional_preconditions = 2;
	repeated RelationshipFilter optional_deletes = 3; // new
}

I like Option 2, because it would not break compatibility of the existing write api, it just adds a single optional field to the WriteRelationshipsRequest message.

What do you think?

henkosch avatar Oct 12 '22 16:10 henkosch

Could also use a oneof, but that would break the generated code (while maintaining API-level compatibility). The other consideration is overlap in the transaction: what happens if you add a relationship in an update, then delete it via the bulk filter?

josephschorr avatar Oct 12 '22 17:10 josephschorr

what happens if you add a relationship in an update, then delete it via the bulk filter?

I think that is why the OPERATION_DELETE's are executed before the OPERATION_CREATE and the OPERATION_TOUCH in the current implementation. See: https://github.com/authzed/spicedb/blob/53c75a218daed0b5ea8d2ccd2fa21c780dbe963b/internal/datastore/postgres/readwrite.go#L125 The final order could be:

  1. Transaction start
  2. Check preconditions
  3. Bulk deletes
  4. Individual deletes (+ Touch)
  5. Creates (+ Touch)
  6. Transaction end

henkosch avatar Oct 12 '22 17:10 henkosch

Actually, that's just an implementation detail. In the current code, if you set an update over the same relationships, an error is raised: https://github.com/authzed/spicedb/blob/main/internal/services/v1/relationships.go#L166

This would have to change for the filter-based approach and we'd have to determine what the ordering meant, if any.

josephschorr avatar Oct 12 '22 17:10 josephschorr