Opus10
Add support for transactional advisory locks
Currently pglock.advisory only supports session-level locks:
with pglock.advisory("my-lock-id"):
# some DB changes
Postgres also supports transaction-level advisory locks:
with transaction.atomic():
local_code._acquire_transaction_level_advisory_lock("my-lock-id")
# some DB changes
...
def _acquire_transaction_level_advisory_lock(
name: str,
nowait: bool = False,
) -> None:
connection = db.connections[using]
cursor = connection.cursor()
lock_id = pglock._core._cast_lock_id(name)
function = f"{'try_' if nowait else ''}advisory_xact_lock"
query = f"SELECT {function}(%s)"
if not connection.in_atomic_block:
raise NotRunningInTransaction()
cursor.execute(query, [lock_id])
lock_acquired = cursor.fetchone()[0]
if nowait and not lock_acquired:
raise AlreadyLocked(f"Could not acquire lock {name}")
It would be great to have support for this here e.g. pglock.advisory(transactional=True).
Transaction-level locks are automatically released when the transaction ends, which is just that bit more robust: with session-level locks (pglock.acquire) there's a risk of leakage if the program crashes before the context manager can exit.
Could we add support for transaction-level locks to pglock.advisory?
The reason why I chose not to support this is because there's no explicit unlocking mechanism for transactional locks, leading to confusing results for users since I can't release the lock at the end of the context manager.
Take this code, for example:
with transaction.atomic():
with pglock.advisory("lock"):
# Do stuff while holding lock
# Even though you're out of the context manager, the lock still remains held until out of the transaction
This can be even more confusing for nested transaction.atomic()s, since the nested ones aren't actually creating new transactions. If, for example, you enable atomic requests in Django, your locks are held for the entire request regardless of where you exited the context manager.
I think the least surprising behavior would be that using transactional=True also starts a transaction with the durable argument. Not sure if that would be too limiting.
Thoughts? Wouldn't mind to support it. Just would hate for people to be bitten by this. Wish postgres supported explicitly unlocking transactional locks
Think that part of the confusion in the example stems from the fact that a context manager creates the illusion that it's defining a new context, and that can't happen here without the unsupported unlock.
Given that, how about making transaction locking a simple function instead of a context manager, so it's more explicit what's happening? Guess that would be somewhat similar to @benthorner's _acquire_transaction_level_advisory_lock.
Given that, how about making transaction locking a simple function instead of a context manager, so it's more explicit what's happening? Guess that would be somewhat similar to @benthorner's _acquire_transaction_level_advisory_lock.
This is how I have implemented a version of transactional advisory locking in the past. As you point out @wesleykendall , it's confusing to rely on the context manager scope in this case. Instead the code can look something like
with atomic():
take_lock('my-lock-id')
...do something
Even then, however, it can perhaps be quite easily missed that the function call is defining some behaviour for the entire duration of the transaction/scope of the atomic context manager. It would be nice if that was more explicit. One thought is to perhaps utilise django-pgtransaction to override atomic directly to add arguments for transactional advisory locking:
import pgtransaction
with pgtransaction.atomic(isolation_level=pgtransaction.SERIALIZABLE, advisory_lock_id='my-lock-id'):
@wesleykendall thanks for taking a look. It's a good point: the scope of a transaction-level advisory lock would not be related to the context manager, which is misleading. In practice, we have done as @mikaraunio and @PaulGilmartin describe: a simple acquire("lock-id") function to acquire a transaction-level advisory lock within some outer transaction.
it can perhaps be quite easily missed that the function call is defining some behaviour for the entire [...] transaction
Over-locking is indeed a potential problem with this approach or the one I originally suggested.
using transactional=True also starts a transaction with the durable argument
This sounds similar to @PaulGilmartin's suggestion i.e. combining locking and atomic() in some way. Using durable makes sense as a way to strongly link the lock to the (outer) transaction. I agree with your concern though: it may not always be appropriate to do both at the same time e.g. if some calling code has already begun a durable transaction.
On balance, my vote would be for e.g.
with transaction.atomic():
pglock.transaction.advisory("lock-id")
# some DB changes
While this carries the risk of over-locking in the outer transaction, it's better than not locking at all - wrapping this Postgres feature makes that easier. It can also be used in any transactional context, giving the most flexibility.
What do you all reckon?
I think those suggestions make sense. I can go ahead and support both modes - make a durable transaction if using the context manager or allow for a functional interface when creating transactional advisory locks.
Let me see what might be the best way to support this in the current interface. I'd like to still use pglock.advisory if possible. Planning to address this in the next release
Nice! Thanks for taking the time to work on this ⭐. I'll go try it out...