refactor(db): use Spanner INSERT OR UPDATE DML#2314
Conversation
chenba
force-pushed
the
refactor/use-spanner-upsert-stor-218
branch
from
c238629 to
efa5104
Compare
chenba
force-pushed
the
refactor/use-spanner-upsert-stor-218
branch
from
efa5104 to
2dbb7dd
Compare
|
|
||
| The entire move (batch commit) happens in one single transaction. Spanner | ||
| limits the number of writes (or "mutations") within a transaction to 20000 | ||
| limits the number of writes (or "mutations") within a transaction to 80000 |
There was a problem hiding this comment.
chenba
force-pushed
the
refactor/use-spanner-upsert-stor-218
branch
from
2dbb7dd to
38caa71
Compare
This commit: - replaces all inserts-and/or-updates with Spanner's upsert DML where appropriate - drops the database_spanner_use_mutations settings as mutations are no longer used
chenba
force-pushed
the
refactor/use-spanner-upsert-stor-218
branch
from
38caa71 to
abc80e1
Compare
taddes
left a comment
taddes
left a comment
There was a problem hiding this comment.
Thanks for doing this! Just a few minor nits.
| by avoiding writing to the batch table, so not included here) | ||
|
|
||
| - Write 1664 (MAX_TOTAL_RECORDS) to `bsos`: max 19968 (1664 * 12) mutations | ||
| - Write 6665 (MAX_TOTAL_RECORDS) to `bsos`: max 79980 (6665 * 12) mutations |
There was a problem hiding this comment.
It'll actually be 6,656 since we're preserving the same legacy pattern, basically 1664 * 4, giving us a tiny bit more headroom, as in the webservices-infra PR. No biggie tho
| - Write 6665 (MAX_TOTAL_RECORDS) to `bsos`: max 79980 (6665 * 12) mutations | |
| - Write 6656 (MAX_TOTAL_RECORDS) to `bsos`: max 79872 (6656 * 12) mutations |
| Totals: | ||
| - quota: False | ||
| 4 + 19968 + 1 = 19973 | ||
| 4 + 79980 + 1 = 79985 |
There was a problem hiding this comment.
| 4 + 79980 + 1 = 79985 | |
| 4 + 79872 + 1 = 79877 |
|
|
||
| - quota: True | ||
| 6 + 19968 + 1 + 6 = 19981 | ||
| 6 + 79980 + 1 + 6 = 79993 |
There was a problem hiding this comment.
| 6 + 79980 + 1 + 6 = 79993 | |
| 6 + 79872 + 1 + 6 = 79885 |
There was a problem hiding this comment.
I'm thinking maybe we should delete this file and rely on the doc you are adding in #2327. No need to duplicate.
There was a problem hiding this comment.
Yeah that works fine I think
pjenvey
left a comment
pjenvey
left a comment
There was a problem hiding this comment.
I'm glad we revisited this, this is great --
It's obvious the benefit of having INSERT OR UPDATE available as DML vs solely in mutations -- the latter being so much more limiting not being intertwined with sql.
The only downside I can see to this change is we're now always reading/writing in the existing data vs not on selective updates (e.g. payload of None/NULL passed in). Is the Spanner query optimizer smart enough to e.g. not penalize an update that's directed to write the existing payload back into the row? (It'd be interesting to see the query execution plan here but I doubt it would answer that).
If not how much are we paying for that? At the very least it's costing us extra mutations, but that's totally worth the cost (we've been limiting ourselves to 20k and that's getting bumped soon anyway).
Approved but I added a couple nits
| bso: params::PostCollectionBso, | ||
| timestamp: SyncTimestamp, | ||
| ) -> DbResult<()> { | ||
| use syncstorage_db_common::util::to_rfc3339; |
There was a problem hiding this comment.
nit: can hoist this import out of here to the module level while you're here -- it was probably more convenient here in the past when this was a test only method
| .unwrap_or_else(null_value); | ||
|
|
||
| let mut row = ListValue::new(); | ||
| row.set_values(RepeatedField::from_vec(vec
|
FYI this also closes STOR-565 |
Good point/question. I'm skeptical that'd be the case. Let's keep an eye on the cost once this is deployed. 🤞 |
Co-authored-by: Taddes <tkorris@mozilla.com>
3 participants
This commit:
Issue(s)
Closes STOR-218