Draft: In case of race condition in content_add, raise SerializationFailure instead of HashCollision.
Partially resolves #2019 (closed), by raising the appropriate exception.
This keeps the retry logic on the client side. I can add it on the server side with a decorator to both functions, but it won't be pretty because it needs to create a new cursor, so the test won't work as-is.
Migrated from D2248 (view on Phabricator)
Merge request reports
Activity
Build is green See https://jenkins.softwareheritage.org/job/DSTO/job/tox/772/ for more details.
Build is green See https://jenkins.softwareheritage.org/job/DSTO/job/tox/773/ for more details.
Thanks for looking into this.
Forcing a serialization issue instead of a HashCollision is interesting, however...
I don't like that this tweaks the transaction isolation level within the function rather than when we open the transaction:
set transaction isolation level
must be the first thing called within the transaction, before any queries happen, so this should be handled within the transaction wrapper.Having to do a bogus select to generate a fake read/write serialization dependency is also quite awful.
While I think serializable is the isolation level that we would want for all queries, I don't know if we really want to eat that cost, for the benefit of "absolute correctness" on one query that fails 1 out of a million times and that we already retry on the client side anyway.
If we really want to go that route, we should also implement a read-only flag, to avoid read queries gratuitously generating serialization dependencies. In addition, the fact that the content_add transactions also include addition to the object storage doesn't play well with the caveats listed in the postgres docs.
Is there anything I'm missing on the status quo on this issue?
! In !288, @olasd wrote: If we really want to go that route, we should also implement a read-only flag, to avoid read queries gratuitously generating serialization dependencies. In addition, the fact that the content_add transactions also include addition to the object storage doesn't play well with the caveats listed in the postgres docs.
As far as I can tell, this particular point should not be an issue anymore, thanks to !360 (closed) (which moved objstorage writes outside the transaction)
mentioned in merge request !351 (closed)
mentioned in issue #2019 (closed)
added mr-reviewed-fall-2023 label
Jenkins job DSTO/gitlab-builds #399 failed .
See Console Output and Coverage Report for more details.