[db] Audit usage of "ON CONFLICT DO NOTHING RETURNING ..."

[smklein]

(here's some background that covers this use-case)

There are a handful of spots in the datastore where we:

• INSERT INTO a table,
• ON CONFLICT, DO NOTHING
• and have a RETURNING clause

Example:

omicron/nexus/src/db/datastore.rs

Lines 880 to 884 in 3ae28c7

	let instance: Instance = diesel::insert_into(dsl::instance)
	.values(instance)
	.on_conflict(dsl::id)
	.do_nothing()
	.returning(Instance::as_returning())

This is valid SQL, but it's a little suspect:

• If there are no conflicts (we're inserting the object with the UUID for the first time) we get the value that we intended to insert.
• However, if there are conflicts, this group of statements does not return any rows. Instead, it results in a NotFound error from the database.

This means that, especially in situations where we expect idempotency, this statement may return different results if invoked multiple times.

[smklein]

Avoiding do_nothing(), and replacing it with do_update().set(...), for any field, seems to be a reasonable workaround?

[davepacheco]

Maybe? The top answer in the linked Stackoverflow post explains a bunch of unfortunate side effects of that, though some of those may not apply in CockroachDB.

I feel like we had a chunk of code for doing exactly this a long time ago. Does that ring a bell?

[smklein]

I googled "ON CONFLICT DO NOTHING RETURNING" and found this; I'm assuming this is what you're referencing?

This answer mentions:

The [approach of updating anyway] seems ok for a single conflict target, few conflicts, small tuples and no triggers.

• Our conflict target is nearly always a UUID, so it's unique
• So there are few conflicts
• This data is relatively small
• CRDB is not using triggers

I do not recall of any code to help in this case of "insert or read the record idempotently".

Their proposal of:

• Use a CTE to generate all the potential values to be inserted
• Attempt to INSERT into the table, DO NOTHING on return
• SELECT the values which were potentially inserted, and...
• ... UNION those values with all the potential values to be inserted

Definitely could also work, and avoid the write contention.

They mention:

The query itself (not counting the side effects) may be a bit more expensive for few dupes, due to the overhead of the CTE and the additional SELECT (which should be cheap since the perfect index is there by definition - a unique constraint is implemented with an index).

FWIW, in our case, we would expect exactly one duplicate.

I do think avoiding any increased write traffic seems like a good goal, but implementing this CTE, including all the necessary type-casting, will probably take a good deal of time to implement for a general case. In the meantime, if these calls are used anywhere that idempotency is expected (e.g., all our saga calls), it's producing wrong results.

We can avoid the do_update().set(...) policy if you're concerned, but I fear that we may avoiding the "optimal" solution in favor of one that is actively wrong.

[davepacheco]

I googled "ON CONFLICT DO NOTHING RETURNING" and found this; I'm assuming this is what you're referencing?

Yeah, sorry, I should have been clearer. This is the link I was talking about. It's the one you included in this issue description.

I do not recall of any code to help in this case of "insert or read the record idempotently".

Ah, I think I was thinking of sql_insert_unique_idempotent_and_fetch(). This went out with PR #192. While looking for this I found I referenced this same StackOverflow answer in RFD 192 as the inspiration for the conditional update CTE. I think some of this probably went into collection_insert...but that's not general enough for what you need here because it also does the collection generation counter stuff. So, nevermind!

I do think avoiding any increased write traffic seems like a good goal, but implementing this CTE, including all the necessary type-casting, will probably take a good deal of time to implement for a general case. In the meantime, if these calls are used anywhere that idempotency is expected (e.g., all our saga calls), it's producing wrong results.

We can avoid the do_update().set(...) policy if you're concerned, but I fear that we may avoiding the "optimal" solution in favor of one that is actively wrong.

👍

[smklein]

Ah, I think I was thinking of sql_insert_unique_idempotent_and_fetch(). This went out with PR #192. While looking for this I found I referenced this same StackOverflow answer in RFD 192 as the inspiration for the conditional update CTE. I think some of this probably went into collection_insert...but that's not general enough for what you need here because it also does the collection generation counter stuff. So, nevermind!

I looked at this implementation to see if I could re-use it, but AFAICT it's doing two operations:

• Inserting with an ON CONFLICT <user provided conflict string> DO NOTHING
• Fetching the row as a follow-up operation

So it's not atomic, and I think there's a comment about this:

omicron/omicron-nexus/src/db/sql_operations.rs

Lines 670 to 674 in 40f1d2f

	/// The insert + fetch sequence is not atomic. It's conceivable that another
	/// client could modify the record in between. The caller is responsible for
	/// ensuring that doesn't happen or does not matter for their purposes. In
	/// practice, the surrounding saga framework should ensure that this doesn't
	/// happen.

(I'm not sure how sagas should help enforce atomicity?)

This operation is easy to do in diesel today; it's basically just:

let result = diesel::insert_into(table)
  .values(values)
  .on_conflict(...)
  .do_nothing()
  .get_result_async()
  .await?;

match result {
  Err(NotFound) => {
    return table.filter(id.eq(id)).get_result_async().await;
  }
  _ => return result;
}

But I think it's the "making it a single atomic operation" part that would be tricky.