#213 Postgres: Add support for transaction-scoped advisory locks with external transactions #222

Tzachi009 · 2024-07-10T12:09:33Z

Add support for for transaction-scoped advisory locks with external transactions.

Related Issue: #213

madelson · 2024-07-22T01:17:51Z

src/DistributedLock.Postgres/PostgresAdvisoryLock.cs

-        Invariant.Require(!UseTransactionScopedLock(connection));
+        // For transaction scoped advisory locks, the lock can only be released by ending the transaction.
+        // If the transaction is internally-owned, then the lock will be released when the transaction is disposed as part of the internal connection management.
+        // If the transaction is externally-owned, then the lock will have to be released explicitly by the transaction initiator.


I'm not comfortable with these semantics; it's just too different from how the other locks work to say that releasing the handle does not release the lock. This feels like the kind of thing that will be hard to discover, since correct-looking code will just be wrong and I don't like having to add except with pg externally-owned-transaction-scoped-locks! to all the generic code examples.

The static utility method feels like a better model for what we're trying to do here, which is to apply a one-way change to a transaction without any notion of a returned disposable scope.

Ok, I can see why a static method in the API may be a better option in this case. I currently have some worries regarding how exactly it will be implemented, but I'll try.

Hi @madelson, I've finally managed to look into the static utility methods. I still didn't add summay comments for the static methods, and I need to revert the change in the ReleaseAsync method in the PostgresAdvisoryLock class, but please take a look at the recent changes and tell me if I am on the right track.

Hi @madelson, do you think you will have time to look into my changes soon?

@Tzachi009 apologies for the long delay. The new static methods look like they're on the right track. I left a few comments.

Hi @madelson, thanks for the feedback.

I replied and fixed to your comments (with the exception of one, where what you asked is not possible).

I reverted my changes in the ReleaseAsync() method in the PostgresAdvisoryLock class.

Added summaries for the static utility methods.

Added some unit tests - I hope it's enough, considering I didn't added a lot of code here.

madelson · 2024-12-03T02:54:54Z

src/DistributedLock.Postgres/PostgresDistributedLock.Extensions.cs

+
+        var handle = DistributedLockHelpers.TryAcquire(PostgresAdvisoryLock.ExclusiveLock, connection, key.ToString(), timeout, cancellationToken);
+
+        return handle != null;


We can use the SyncViaAsync class to unify the sync/async implementations here. E.g. I think this method should just be:

return SyncViaAsync.Run( state => TryAcquireWithTransactionAsync(state.key, state.transaction, state.timeout, state.cancellationToken), (key, transaction, timeout, cancellationToken);

madelson · 2024-12-03T02:57:34Z

src/DistributedLock.Core/Internal/DistributedLockHelpers.cs

+            state => TryAcquireAsync(strategy, connection, resourceName, timeout, cancellationToken),
+            (strategy, connection, resourceName, timeout, cancellationToken)
+        );
+    #endregion


Let's remove these helper methods. They're just calling strategy.TryAcquire(Async) which can be called directly by PostgresDistributedLock

madelson · 2024-12-03T03:04:04Z

src/DistributedLock.Postgres/PostgresDistributedLock.Extensions.cs

+
+        var connection = new PostgresDatabaseConnection(transaction);
+
+        return DistributedLockHelpers.AcquireAsync(PostgresAdvisoryLock.ExclusiveLock, connection, key.ToString(), timeout, cancellationToken).ConvertToVoid();


I'd expect the implementation of this to be something like:

return TryAcquireWithTransactionAsync(...).ThrowTimeoutIfFalse(); // currently private in DistributedLockHelpers, can be made public

Added a call to ThrowTimeoutIfNull(), it was previsouly part of the helper method that I just removed

madelson · 2024-12-03T03:04:35Z

src/DistributedLock.Postgres/PostgresDistributedLock.Extensions.cs

+
+        async ValueTask<bool> TryAcquireAsync()
+        {
+            var handle = await DistributedLockHelpers.TryAcquireAsync(PostgresAdvisoryLock.ExclusiveLock, connection, key.ToString(), timeout, cancellationToken);


All awaits need ConfigureAwait(false)

madelson · 2024-12-03T03:07:47Z

src/DistributedLock.Postgres/PostgresDistributedLock.Extensions.cs

+
+        async ValueTask<bool> TryAcquireAsync()
+        {
+            var handle = await DistributedLockHelpers.TryAcquireAsync(PostgresAdvisoryLock.ExclusiveLock, connection, key.ToString(), timeout, cancellationToken);


I think we want to call await handle.DisposeAsync().ConfigureAwait(false); here with a comment saying that for an externally-owned transaction the release is a noop but we want to dispose proactively to prevent the handle's managed finalizer (see ManagedFinalizerQueue) from running.

I am afraid that's not possible because of how the code is written. I actually tried to avoid this exact scenario, since disposal of the handle would have reached the ReleaseAsync method in the PostgresAdvisoryLock class - where you didn't like the changes I did there. Instead, I am calling the PostgresAdvisoryLock class directly in order to acquire the lock and get an object cookie in return, which I ignore. Releasing the handle would have required to go through the DedicatedConnectionOrTransactionDbDistributedLock class.

madelson · 2024-12-03T03:09:49Z

src/DistributedLock.Postgres/PostgresDistributedLock.Extensions.cs

+        if (key == null) { throw new ArgumentNullException(nameof(key)); }
+        if (transaction == null) { throw new ArgumentNullException(nameof(transaction)); }
+
+        var connection = new PostgresDatabaseConnection(transaction);


I feel like we can dispose this object after the acquire operation (just move the creation inside the helper function and add an async using block).

I agree, it shouldn't impact anything regarding the external connection, so added disposal

madelson · 2024-12-31T02:11:18Z

src/DistributedLock.Postgres/PostgresDistributedLock.Extensions.cs

+    public static ValueTask AcquireWithTransactionAsync(PostgresAdvisoryLockKey key, IDbTransaction transaction, TimeSpan? timeout = null, CancellationToken cancellationToken = default) =>
+        AcquireWithTransactionAsyncInternal(key, transaction, timeout, cancellationToken);
+
+    internal static ValueTask<bool> TryAcquireWithTransactionAsyncInternal(PostgresAdvisoryLockKey key, IDbTransaction transaction, TimeSpan timeout = default, CancellationToken cancellationToken = default)


This internal method shouldn't have default parameter values

madelson · 2024-12-31T02:23:57Z

src/DistributedLock.Postgres/PostgresDistributedLock.Extensions.cs

+
+            await using (connection.ConfigureAwait(false))
+            {
+                var handle = await PostgresAdvisoryLock.ExclusiveLock.TryAcquireAsync(connection, key.ToString(), timeout, cancellationToken).ConfigureAwait(false);


If I'm reading the code correctly, this isn't actually a handle but merely a "Cookie" object which for Postgres is just a sentinel value.

Let's call this var lockAcquiredCookie to avoid confusion.

That's correct, I changed the name

madelson · 2025-01-04T20:47:35Z

src/DistributedLock.Postgres/PostgresAdvisoryLock.cs

+        if (!connection.IsExernallyOwned) { return connection.HasTransaction; }
+
+        // If the connection is externally-owned with an established transaction, we don't want to pollute it with a save point 
+        // which we won't be able to release in case the lock will be acquired.


Thinking about this more, I'm not sure I agree with the logic for two reasons.

Why would a save point pollute the external transaction? We always roll back the save point regardless of lock acquisition and this happens right after we succeed/fail to acquire. My understanding is that a savepoint rollback will not prevent us from holding onto an acquired lock. See calls to RollBackTransactionTimeoutVariablesIfNeededAsync

In the catch statement on 198/201, we return true which is the opposite behavior for what should be the same scenario (external connection with transaction).

The problem with not having a save point is that we end up polluting the transaction by setting the statement_timeout and lock_timeout values; the point of the savepoint is to clean those up before we return to the caller.

I added more details to the comments in this method in the new PR, I hope it's clearer.

I think it's inaccurate to say that we always roll back the save point regardless of lock acquisition. Before my changes, there was an if statement (which is still there, just changed the comment above it) in RollBackTransactionTimeoutVariablesIfNeededAsync that checks if the lock has been acquired and if the lock is transactional - if that's the case, then we can't rollback a savepoint, since it will release the lock (it's true, I checked), therefore there is no point in creating a save point. Although, you are right when it comes to polluting the transaction by setting the statement_timeout and lock_timeout values in this scenario - how do you think we should handle it, if at all? A warning in the public comments of the API will suffice? Or should we write code that try to restore the previous values?

You can only reach the try/catch statement if someone sends an external connection via the existing API - this statement already existed before my changes. I didn't change it and I don't think that we should. I prevented external connections that come through the new transactional APIs from getting there with the previous if statement - if (connection.HasTransaction) { return false; }.

RollBackTransactionTimeoutVariablesIfNeededAsync that checks if the lock has been acquired and if the lock is transactional - if that's the case, then we can't rollback a savepoint

You are correct. Unfortunately this creates another problem which is that we are now polluting the external connection's statement_timeout and lock_timeout state.

So for the case of an externally-owned transaction, I think we need some code that does this:

Reads the initial settings with code like this:

SELECT current_setting('statement_timeout') AS statement_timeout, current_setting('lock_timeout') AS lock_timeout;

In RollBackTransactionTimeoutVariablesIfNeededAsync, we can pass in these values. In that function it would do something like this:

if (originalTimeoutValues is { } timeoutValues) { // run query to SET LOCAL restore the original values return; }

Thinking about it, I wonder if we should just drop the SAVEPOINT logic altogether in favor of this. If so, we could replace ShouldDefineSavePoint and RollBackTransactionTimeoutVariablesIfNeededAsync with something like this:

private async ValueTask<CapturedTimeoutSettings?> CaptureTimeoutSettingsIfNeededAsync(...) { // returns null if the connection does not have an implicit or explicit transaction } private async ValueTask RestoreTimeoutSettingsAsync(CapturedTimeoutSettings? settings) { if (settings is null) { return; } // issue a command with 2 SET LOCAL to restore the settings }

I think it would be good to add a unit test case for this in the case of the new API. Something like:

Start transaction

SET LOCAL on lock_timeout and statement_timeout

Acquire lock

Verify that lock_timeout and statement_timeout are unchanged

All right, I will start looking into replacing save point logic

madelson · 2025-01-04T20:51:15Z

src/DistributedLock.Postgres/PostgresAdvisoryLock.cs

-        !connection.IsExernallyOwned && connection.HasTransaction;
+        // Transaction-scoped locking is supported on both externally-owned and internally-owned connections,
+        // as long as the connection has a transaction.
+        connection.HasTransaction;


I think this is right but the comment is somewhat misleading. My understanding is that there is no path to get here with an external connection that explicitly has a transaction (vs. implicitly which is tested for above) except in the case where the caller deliberately went through one of the transactional locking APIs. Do you concur?

If so, let's be clear about that.

I concur and changed the comment in the new PR, I hope it's clearer

madelson · 2025-01-04T20:51:50Z

@Tzachi009 I merged this into the next release branch but I may have acted too quickly as I now think there is an issue with the implementation: https://github.com/madelson/DistributedLock/pull/222/files#r1903151395

Also, https://github.com/madelson/DistributedLock/pull/222/files#r1903151800

Mind re-submitting against release-2.6?

Tzachi009 · 2025-01-07T07:31:38Z

@madelson I re-submitted the PR against release-2.6 - #235

Add initial support locking with external transaction for Postgres

4414996

Tzachi009 mentioned this pull request Jul 10, 2024

Postgres: Add support for transaction-scoped advisory locks with external DB connections #213

Closed

madelson reviewed Jul 22, 2024

View reviewed changes

Add support for static utility methods

ad1c932

madelson reviewed Dec 3, 2024

View reviewed changes

Fixes after CR + unit tests

056fa15

Tzachi009 marked this pull request as ready for review December 18, 2024 14:16

madelson reviewed Dec 31, 2024

View reviewed changes

Changes after CR

a56a7c7

madelson added this to the 2.6 milestone Jan 4, 2025

madelson changed the base branch from master to release-2.6 January 4, 2025 20:00

madelson merged commit 324e17c into madelson:release-2.6 Jan 4, 2025
1 check failed

madelson reviewed Jan 4, 2025

View reviewed changes

Tzachi009 mentioned this pull request Jan 7, 2025

#213 Postgres: Add support for transaction-scoped advisory locks with external transactions - Continuation #235

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#213 Postgres: Add support for transaction-scoped advisory locks with external transactions #222

#213 Postgres: Add support for transaction-scoped advisory locks with external transactions #222

Tzachi009 commented Jul 10, 2024

madelson Jul 22, 2024

Tzachi009 Jul 23, 2024

Tzachi009 Aug 18, 2024

Tzachi009 Sep 26, 2024

madelson Dec 3, 2024

Tzachi009 Dec 18, 2024

madelson Dec 3, 2024 •

edited

Loading

Tzachi009 Dec 18, 2024

madelson Dec 3, 2024

Tzachi009 Dec 18, 2024

madelson Dec 3, 2024

Tzachi009 Dec 18, 2024

madelson Dec 3, 2024

Tzachi009 Dec 18, 2024

madelson Dec 3, 2024

Tzachi009 Dec 18, 2024

madelson Dec 3, 2024

Tzachi009 Dec 18, 2024

madelson Dec 31, 2024

Tzachi009 Dec 31, 2024

madelson Dec 31, 2024

Tzachi009 Dec 31, 2024

madelson Jan 4, 2025 •

edited

Loading

Tzachi009 Jan 7, 2025 •

edited

Loading

madelson Jan 8, 2025

Tzachi009 Jan 9, 2025

madelson Jan 4, 2025

Tzachi009 Jan 7, 2025 •

edited

Loading

madelson commented Jan 4, 2025

Tzachi009 commented Jan 7, 2025


		var handle = DistributedLockHelpers.TryAcquire(PostgresAdvisoryLock.ExclusiveLock, connection, key.ToString(), timeout, cancellationToken);

		return handle != null;


		var connection = new PostgresDatabaseConnection(transaction);

		return DistributedLockHelpers.AcquireAsync(PostgresAdvisoryLock.ExclusiveLock, connection, key.ToString(), timeout, cancellationToken).ConvertToVoid();

#213 Postgres: Add support for transaction-scoped advisory locks with external transactions #222

#213 Postgres: Add support for transaction-scoped advisory locks with external transactions #222

Conversation

Tzachi009 commented Jul 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madelson Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madelson Jan 4, 2025 • edited Loading

Choose a reason for hiding this comment

Tzachi009 Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tzachi009 Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

madelson commented Jan 4, 2025

Tzachi009 commented Jan 7, 2025

madelson Dec 3, 2024 •

edited

Loading

madelson Jan 4, 2025 •

edited

Loading

Tzachi009 Jan 7, 2025 •

edited

Loading

Tzachi009 Jan 7, 2025 •

edited

Loading