Remove redundant same-shard check for cross-shard updates

[sgrif]

We don't need to check this in insert_row, we already checked if we're on the same shard in UpdateMulti::execute_internal before calling this

[codecov]

Codecov Report

❌ Patch coverage is 83.33333% with 4 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
.../frontend/client/query_engine/multi_step/update.rs	83.33%	4 Missing ⚠️

📢 Thoughts on this report? Let us know!

[levkk]

Could you point me to the check? Also, we have a conflicting PR in flight to support multi-row updates #1109

[sgrif]

UpdateMulti::execute_internal

[sgrif]

I don't think this should cause problems with the other PR, the same-shard check in that PR is redundant as best I can tell

[levkk]

I think this check is there to prevent the trivial case where the the sharding key is in the where-clause. But, the other check also checks for this:

UPDATE users SET sharding_key = $5 WHERE banned = true;

We check if the rows returned by the WHERE predicate are on the same shard after fetching them from the database. Edge case, but still valid.

[sgrif]

Do you think it's worth the added complexity (especially in that other PR which makes this check even more complex) to avoid doing a multi-step update on edge cases like that? Personally I'm fine with it just doing the select/delete/insert in cases where we can't statically determine the shard from the where clause, but I don't feel super strongly

[levkk]

I think so yeah. You could add a comment there as part of this PR and think about it some more - definitely don't want to lose this context. The more complexity we handle, the less the user has to worry about.

[sgrif]

I don't think it's something the user has to worry about. The end result is the same either way, it's purely a performance optimization, not a semantic one

[levkk]

True. But yeah, we should keep this imo, especially since we can't do all sharding key updates yet.

[sgrif]

Okie dokie

[sgrif]

(Gonna try and come back and add an explicit test case for this when I have time)