Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optionally return operation statuses on WriteRelationships #1903

Open
benvernier-sc opened this issue May 17, 2024 · 3 comments
Open

Optionally return operation statuses on WriteRelationships #1903

benvernier-sc opened this issue May 17, 2024 · 3 comments
Labels
kind/proposal Something fundamentally needs to change

Comments

@benvernier-sc
Copy link

Problem Statement

The SpiceDB docs cover a Two writes & commits scenario to do dual writes between an existing system and SpiceDB. Those docs suggest performing a WriteRelationships call as part of the primary write path for the data, then performing a DeleteRelationships request if a rollback if necessary.

The issue is that if I had a relationship that already existed, I performed a WriteRelationships request with a TOUCH operation on that existing relationship (which would have been a no-op internally) then want to rollback my operation, calling DeleteRelationships would actually leave my system in a different state than what it was in before I started the operation.

Similarly, if one of the operations of my WriteRelationships request was a DELETE operation but the relationship didn't actually exist in SpiceDB, I do not want to create the relationship on rollback as once again this would leave the system in a different state.

Solution Brainstorm

When making a call to WriteRelationships (with either a single or multiple updates), there are cases where it would be useful to be able to get what the status of each operation was in the response.

In particular, knowing whether a TOUCH operation actually created a relationship or if it was a no-op, and knowing whether a DELETE operation actually deleted a relationship or if it was a no-op.

I acknowledge that systematically returning that information might come at a performance cost that existing use cases that don't need this information might not be comfortable with, so this could be an option set on the request to specify whether the status breakdown should be returned.

@benvernier-sc
Copy link
Author

@josephschorr I did a quick PoC for this:

Let me know your thoughts, if you're happy with the approach I can extend the implementation to cover the other datastores and actually raise a PR for the change.

@josephschorr
Copy link
Member

@benvernier-sc Seems reasonable at first glance, although we'd need to clean up a bit the conditionals used around whether to add the RETURNING on not.

I did a quick check through the other datastores:

  • MySQL: uses select for update, so should be trivial to determine which rows were changed
  • CRDB: supports returning so should be similar to the Postgres driver changes, although it will have the same performance impact
  • Memdb: doesn't matter the performance impact, and trivial to implement
  • Spanner: ⚠️ PROBLEM - I'm unsure if we can do it in Spanner, which presents a problem around API compatibility. This will require more research

@benvernier-sc
Copy link
Author

although we'd need to clean up a bit the conditionals used around whether to add the RETURNING on not.

Did you have something specific in mind? The way I could see to clean it up would be to do something like

columnsToReturn := []string{
	colNamespace,
	colObjectID,
	colRelation,
	colUsersetNamespace,
	colUsersetObjectID,
	colUsersetRelation,
}

if returnStatus {
	columnsToReturn = append(columnsToReturn,
		colCaveatContextName,
		colCaveatContext,
	)
}

builder := deleteTuple.
		Where(deleteClauses).
		Suffix(fmt.Sprintf("RETURNING %s", strings.Join(",", columnsToReturn)))

Spanner: ⚠️ PROBLEM - I'm unsure if we can do it in Spanner, which presents a problem around API compatibility. This will require more research

From a quick look, it does seem like it's going to be hard to do in Spanner with the current BufferWrite as we don't get any information back. There would be a way to achieve it by using SQL in a spanner.Statement and executing it through rwt.spannerRWT.Query() that returns a *spanner.RowIterator, but that would be a pretty significant refactor (some official examples). It also then leads to the question of whether we always use Query, or if we continue using BufferWrite when we don't need the statuses and use Query when we do, but that becomes two separate implementations to maintain over time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/proposal Something fundamentally needs to change
Projects
None yet
Development

No branches or pull requests

2 participants