Implement support for metadata associated with read-write transactions #1914

josephschorr · 2024-05-26T23:39:39Z

This will allow callers of APIs such as WriteRelationships and DeleteRelationships to assign metadata to the transaction that will be mirrored back out in the Watch API, to provide a means for correlating updates

First part of #966

Each datastore has a different means of correlating between the transaction and the associated change(s) coming out of the watch API:

Memdb: The metadata is stored on the changes struct
Postgres: A metadata column was added to the transactions table
MySQL: A metadata column was added to the transactions table
Spanner: A transaction tag is used to correlate the transaction and the metadata is stored in an auto-GC-ing table, indexed by the transaction tag
CRDB: A row is inserted into a metadata table as part of the transaction, which includes the metadata. When seen at the same revision as the other changes, the metadata becomes correlated with that transaction. The rows are set to auto-GC after a period of time

github-actions · 2024-05-26T23:40:15Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

josephschorr · 2024-05-26T23:40:54Z

I have read the CLA Document and I hereby sign the CLA

josephschorr · 2024-05-26T23:41:10Z

recheck

vroldanbet

Left a few questions and suggestions, I think the approach makes sense to me. The main concerns are how does this affect write-throughput, but at first glance it shouldn't introduce any new hotspots.

A potential issue here is the overlap with the concept of tuple-level metadata, but we've certainly heard more about this use case than tuple-level metadata. I also do wonder if folks will eventually ask for a transactions API to read these back, but that's an easy thing to do if the time comes.

Finally: I understand these are just the Datastore API changes, but we'd have to make sure there are limits put in place so that folks don't abuse that new field.

vroldanbet · 2024-05-28T08:47:46Z

internal/datastore/crdb/crdb.go

+		// If metadata is to be attached, write that row now.
+		if config.Metadata != nil {
+			randomKey := uuid.NewString()
+			expiresAt := time.Now().Add(25 * time.Hour)


Why 25? The migration has the TTL set to @daily.

The issue I see here is that the metadata TTL is not aligned with the relationship GC Window.

if longer than the GC window, we store rows in the DB that cannot be retrieved.

if shorter than the GC window, clients may ask for a revision and fail to obtain the metadata because it had expired earlier.

IMO we should set expiresAt to the value of the server GC window parameter. The risk is the user changes the value to a value distinct from the default, and the table TTL configuration drifts. This is a better scenario IMO than hard coding because at least a human operator can go and modify the table directly: not ideal, but at least there is an avenue to fix it. Additionally, we could log on startup when the configured GC window and the TTL on the table have drifted, and prompt the user to intervene manually.

Updated to gcWindow + 1 minute

vroldanbet · 2024-05-28T08:53:20Z

internal/datastore/crdb/crdb.go

@@ -310,6 +312,23 @@ func (cds *crdbDatastore) ReadWriteTx(
 			Executor: pgxcommon.NewPGXExecutor(querier),
 		}

+		// If metadata is to be attached, write that row now.
+		if config.Metadata != nil {
+			randomKey := uuid.NewString()


I suspect we should use the native UUID type in CRDB, which is likely more space-efficient than a VARCHAR, and have the database generate it. See https://www.cockroachlabs.com/docs/stable/uuid

vroldanbet · 2024-05-28T08:54:58Z

internal/datastore/crdb/migrations/zz_migration.0007_add_transaction_metadata_table.go

+const (
+	addTransactionMetadataTableQuery = `
+		CREATE TABLE transaction_metadata (
+			key VARCHAR PRIMARY KEY,


I'd suggest using the UUID type, and set the default value to generate a UUID. That way we don't have to send the UUID over the wire on each insert.

vroldanbet · 2024-05-28T08:57:21Z

internal/datastore/mysql/query_builder.go

@@ -99,8 +99,8 @@ func getLastRevision(tableTransaction string) sq.SelectBuilder {
 	return sb.Select("MAX(id)").From(tableTransaction).Limit(1)
 }

-func getRevisionRange(tableTransaction string) sq.SelectBuilder {
-	return sb.Select("MIN(id)", "MAX(id)").From(tableTransaction)
+func loadRevisionRange(tableTransaction string) sq.SelectBuilder {


why did the SQL change here?

It wasn't being used so I just repurposed the name

vroldanbet · 2024-05-28T09:00:10Z

internal/datastore/postgres/revisions.go

 	ctx, span := tracer.Start(ctx, "createNewTransaction")
 	defer span.End()

-	cterr := tx.QueryRow(ctx, createTxn).Scan(&newXID, &newSnapshot)
+	if metadata == nil {
+		metadata = map[string]any{}


let's create a global var for this so we don't allocate (I'm actually unsure if this allocates, but just in case)

vroldanbet · 2024-05-28T09:12:34Z

internal/datastore/spanner/migrations/zz_migration.0010_add_transaction_metadata_table.go

+
+const (
+	addTransactionMetadataTable = `CREATE TABLE transaction_metadata (
+			transaction_tag STRING(MAX) NOT NULL,


if the tag is a UUID, use STRING(36) as the documentation suggests: https://cloud.google.com/spanner/docs/primary-key-default-value#universally_unique_identifier_uuid

vroldanbet · 2024-05-28T09:13:21Z

internal/datastore/spanner/migrations/zz_migration.0010_add_transaction_metadata_table.go

+			created_at TIMESTAMP DEFAULT (CURRENT_TIMESTAMP()),
+			metadata JSON
+		) PRIMARY KEY (transaction_tag),
+		ROW DELETION POLICY (OLDER_THAN(created_at, INTERVAL 2 DAY))


same feedback as with CRDB around TTL configuration. Does interval 2 day means "GC every second day"?

It means after 2 days

vroldanbet · 2024-05-28T09:16:41Z

internal/datastore/spanner/spanner.go

 func (sd *spannerDatastore) ReadWriteTx(ctx context.Context, fn datastore.TxUserFunc, opts ...options.RWTOptionsOption) (datastore.Revision, error) {
 	config := options.NewRWTOptionsWithOptions(opts...)

 	ctx, span := tracer.Start(ctx, "ReadWriteTx")
 	defer span.End()

+	transactionTag := fmt.Sprintf("spicedb-%s", uuid.NewString())


concatenate instead of fmt. Also why prepending spicedb-, seems unnecessary waste and UUID should guarantee they are unique, and we already correlate them with the table.

Because we might end up using transaction tags of other forms, so I want a prefix

vroldanbet · 2024-05-28T09:19:57Z

internal/datastore/spanner/spanner.go

@@ -252,15 +285,15 @@ func (sd *spannerDatastore) ReadWriteTx(ctx context.Context, fn datastore.TxUser
 		}

 		return nil
-	})
+	}, spanner.TransactionOptions{TransactionTag: transactionTag})


any specific limitations around tagging a transaction? e.g. number of them? How long do they stay around in the DB? Do we have an understanding of how it affects write throughput?

Should we check if config.Metadata is present outside of the RTW not to tag transactions unless requested by the customer? I assume tagging is not free, and not every Spanner user may be using transaction metadata feature.

It shouldn't have any impact, since I believe Spanner just sets it to empty by default

vroldanbet · 2024-05-28T09:25:48Z

internal/datastore/common/changes.go

+
+	record := ch.recordForRevision(rev)
+	if len(record.metadata) > 0 {
+		log.Ctx(ctx).Warn().Msg("metadata already set for revision")


superfluous log entry as we are going to log the error anyway

This will allow callers of APIs such as WriteRelationships and DeleteRelationships to assign metadata to the transaction that will be mirrored back out in the Watch API, to provide a means for correlating updates

josephschorr · 2024-06-01T22:37:09Z

Updated

github-actions bot added area/datastore Affects the storage system area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) labels May 26, 2024

authzedbot added a commit to authzed/cla that referenced this pull request May 26, 2024

@josephschorr has signed the CLA in authzed/spicedb#1914

670bfd6

josephschorr mentioned this pull request May 27, 2024

PROPOSAL: Add optional watch annotations to RelationshipUpdate #966

Open

vroldanbet reviewed May 28, 2024

View reviewed changes

josephschorr added 2 commits June 1, 2024 18:36

Implement support for metadata associated with read-write transactions

f4a9671

This will allow callers of APIs such as WriteRelationships and DeleteRelationships to assign metadata to the transaction that will be mirrored back out in the Watch API, to provide a means for correlating updates

Move e2e to CRDB v22.2.19

86836c8

josephschorr force-pushed the rw-tx-metadata branch from cb0d605 to 86836c8 Compare June 1, 2024 22:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement support for metadata associated with read-write transactions #1914

Implement support for metadata associated with read-write transactions #1914

josephschorr commented May 26, 2024 •

edited

Loading

github-actions bot commented May 26, 2024 •

edited

Loading

josephschorr commented May 26, 2024

josephschorr commented May 26, 2024

vroldanbet left a comment

vroldanbet May 28, 2024

josephschorr Jun 1, 2024

vroldanbet May 28, 2024

vroldanbet May 28, 2024

vroldanbet May 28, 2024

josephschorr Jun 1, 2024

vroldanbet May 28, 2024

vroldanbet May 28, 2024

vroldanbet May 28, 2024

josephschorr Jun 1, 2024

vroldanbet May 28, 2024

josephschorr Jun 1, 2024

vroldanbet May 28, 2024

josephschorr Jun 1, 2024

vroldanbet May 28, 2024

josephschorr commented Jun 1, 2024

Implement support for metadata associated with read-write transactions #1914

Are you sure you want to change the base?

Implement support for metadata associated with read-write transactions #1914

Conversation

josephschorr commented May 26, 2024 • edited Loading

github-actions bot commented May 26, 2024 • edited Loading

josephschorr commented May 26, 2024

josephschorr commented May 26, 2024

vroldanbet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephschorr commented Jun 1, 2024

josephschorr commented May 26, 2024 •

edited

Loading

github-actions bot commented May 26, 2024 •

edited

Loading