Andy Salnikov, this is a tiny change, but because it's a subtle concurrency-related change, I'm hoping you can actually take a look at Database.sync in its entirety, and see if you agree with my conclusion that the outcome of a race condition that occurs before, after, or during it but within the same (possibly outer) transaction is either:

• one processing "winning"
• an exception

i.e. not any kind of silently bad write.

PR is here: https://github.com/lsst/daf_butler/pull/579

I'm going to repurpose DM-27224 for fixing the other rarer deadlocks noted in my last post; I'll comment there with my ideas.

I think dropping the table lock is the right thing to do here. I am not 100% sure that this solves all issues though, my reading of everything makes me think that a lot of it depends on transaction isolation and if that is not handled consistently we may still get into trouble. One note for sync() is that there may be couple of other ways to implement it:

• SELECT ... FOR UPDATE first to put a lock on a record (or index) and INSERT/UPDATE after that
• SELECT followed by INSERT/UPDATE+COMMIT, and if that fails do another SELECT, but this needs to be done in a separate transaction obviously.

Just one more comment - removing the table lock should greatly reduce chance of a deadlock, but it does not eliminate it completely. When doing INSERT/UPDATE some potion of a table (or its index) still needs to be locked (existing read lock promoted to exclusive). If two concurrent clients want to insert/update the same or neighbor rows we may still get into a situation with a deadlock.

I do think this logic should be safe enough under any isolation level (other than READ UNCOMMITTED, which isn't really useful for anything), but here are the caveats I'm aware of:

• With READ COMMITTED (the default in PostgreSQL, and what we are using there), using sync at the start of a transaction doesn't mean its guarantees persist to the end of the transaction, as code calling sync often assumes (e.g. sync an "instrument" record, then follow up by inserting "detector" records assuming the instrument hasn't changed).  But I think that's just the usual guarantee of READ COMMITTED, and our exposure to actual problems here is pretty small, because foreign keys will prevent any remotely probable invalid inserts.
• With REPEATABLE READ or SERIALIZABLE isolation, sync's guarantees do persist until the end of the current transaction, but one can get serialization errors from the database when conflicting concurrent writes occur. That problem can already happen with READ COMMITTED if the concurrent write happens between the INSERT ... ON CONFLICT IGNORE and SELECT run by sync, and if it does we raise RuntimeError (or, as you noted, another deadlock exception).  With the stronger-guarantee isolation levels, the type of exception raised would change to something low-level emitted by SQLAlchemy, and it would also be possible to see that after sync ends but before its transaction is committed.

I think it might be worth trying to use SERIALIZABLE isolation to resolve the first point in the future; there are two challenges:

• if it yields database exceptions for possibly conflicting writes that don't actually conflict (especially those INSERT ... ON CONFLICT IGNORE statements we rely on), we'd need to do retries, and that's a big restructuring (this was the original idea for DM-27224)
• we can't change the isolation level after a transaction has begun, so we'd need to either make it illegal to use sync inside a transaction again, use SERIALIZABLE isolation by default, or demand that higher-level code (e.g. in obs_base) explicitly mark transactions that need to be started with SERIALIZABLE.

I'll note on DM-27224 that there are comments here that should be taken into account (your comments and this last one by me, at least) when deciding what to do in the long term, and merge this fix now.

Marking preops because it was backported for DP0.2