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.

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);

Sure, done

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

Removed

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

All awaits need ConfigureAwait(false)

Yep, added

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.

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

This internal method shouldn't have default parameter values

Done

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

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

1. 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
2. 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.

1. 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?
2. 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:

1. Reads the initial settings with code like this:

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

2. 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:

1. Start transaction
2. SET LOCAL on lock_timeout and statement_timeout
3. Acquire lock
4. Verify that lock_timeout and statement_timeout are unchanged

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

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