feat(datastore): idempotent (TOUCH-like) bulk relationship import#3195
feat(datastore): idempotent (TOUCH-like) bulk relationship import#3195poucet wants to merge 1 commit into
Conversation
|
CLA Assistant Lite bot: I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request |
b1aeb0a to
cb278e1
Compare
ImportBulkRelationships routes through the datastore BulkLoad, which used strict CREATE semantics: importing a relationship that already existed aborted the load with an AlreadyExists error. Make BulkLoad idempotent (TOUCH-like) across all datastores, so already-existing relationships are silently skipped rather than failing the load, which makes re-imports idempotent. Per datastore: - postgres/crdb: replace the shared CopyFrom-based BulkLoad with batched INSERT ... ON CONFLICT DO NOTHING. Bare ON CONFLICT DO NOTHING works for both backends (Postgres infers the uq_relation_tuple_living_xid partial unique index; CockroachDB infers pk_relation_tuple). Rows are batched to stay under the 65535 bind-parameter wire limit. - mysql: use INSERT IGNORE. - memdb: skip relationships that already exist when scanning the source. - spanner: use an InsertOrUpdate mutation instead of Insert. Returned count semantics: BulkLoad now returns the number of relationships actually inserted (excluding skipped duplicates) for postgres, crdb, mysql, and memdb. Spanner cannot cheaply distinguish new from existing rows (mutations are applied blindly at commit), so it continues to report the number processed; this is documented at the call site. Testing: - Add BulkUploadIdempotentTest to the shared datastore suite, asserting the no-duplicate invariant via queries (holds for every datastore) plus the initial-load count. - Remove the now-obsolete BulkUploadAlreadyExistsError / BulkUploadAlreadyExistsSameCallError tests and the bulk-load portion of CreateAlreadyExistingTest, which asserted the removed error-on-duplicate behavior.
cb278e1 to
ed2bf8c
Compare
|
We considered this when writing the For If you aren't using golang, we'd welcome a PR to the client library you're using. I've put up a draft PR of one in Java here: authzed/authzed-java#174, though with the caveat that I'm not well-versed in writing idiomatic Java and I'd welcome feedback on it if that's the direction y'all would be going. |
|
What about making this an option on the ImportBulkRelationships request? If I want to load a lot of data into my spicedb, then it would be nice if that load was idempotent if it failed half-way through. |
|
How would that option work? Are you talking about the individual messages that are sent over the wire or the overall request or? I agree that |
|
I was considering a singular boolean option at the request level to determine whether to go with the existing mechanism or the one I introduced in this PR. |
Summary
ImportBulkRelationshipsroutes through the datastoreBulkLoad, which used strict CREATE semantics: importing a relationship that already existed aborted the whole load with anAlreadyExistserror.This PR makes
BulkLoadidempotent (TOUCH-like) across all datastores — already-existing relationships are silently skipped instead of failing the load, so re-imports are idempotent.Changes per datastore
pgxcommon.BulkLoadCopyFromimplementation with batchedINSERT ... ON CONFLICT DO NOTHING. BareON CONFLICT DO NOTHINGworks for both (Postgres infers theuq_relation_tuple_living_xidpartial unique index — the same arbiterWriteRelationshipsTOUCH relies on; CockroachDB inferspk_relation_tuple). Rows are batched under the 65535 bind-parameter wire limit.INSERT IGNORE(already an idiom in this codebase).InsertOrUpdatemutation instead ofInsert.Returned count (
num_loaded)BulkLoadnow returns the number of relationships actually inserted (excluding skipped duplicates) for Postgres, CockroachDB, MySQL, and memdb. Spanner cannot cheaply distinguish new from existing rows (mutations are applied blindly at commit), so it continues to report the number processed; this is documented at the call site.Tests
BulkUploadIdempotentTestto the shared datastore conformance suite (AllWithExceptions), so it runs for every datastore. It asserts the no-duplicate invariant via queries (universally true) plus the initial-load count, and covers re-load, superset-load, and same-transaction duplicate cases.BulkUploadAlreadyExistsError/BulkUploadAlreadyExistsSameCallErrortests and the bulk-load portion ofCreateAlreadyExistingTest, which asserted the removed error-on-duplicate behavior.Behavior / compatibility notes
AlreadyExists; those rows are skipped on every datastore.InsertOrUpdatedoes overwrite, a minor cross-store difference noted in code.COPYthroughput for upsert-style batched inserts. This is inherent to conflict tolerance —COPYcannot expressON CONFLICT, and aCOPY-into-temp-table approach was rejected because CockroachDB temp tables are experimental/off-by-default.Testing performed
Per-datastore bulk suites +
TestCreateAlreadyExistingpass on memdb, Postgres (both configs), CockroachDB (integrity + non-integrity), MySQL, and Spanner. Full memdb suite and the service-levelImportBulkRelationshipstest also pass.