Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
madelson merged 4 commits into from
Jan 4, 2025
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4414996
Add initial support locking with external transaction for Postgres
Tzachi009 Jul 10, 2024
ad1c932
Add support for static utility methods
Tzachi009 Aug 18, 2024
056fa15
Fixes after CR + unit tests
Tzachi009 Dec 18, 2024
a56a7c7
Changes after CR
Tzachi009 Dec 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 25 additions & 16 deletions src/DistributedLock.Postgres/PostgresAdvisoryLock.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,12 @@ private PostgresAdvisoryLock(bool isShared)

// Our acquire command will use SET LOCAL to set up statement timeouts. This lasts until the end
// of the current transaction instead of just the current batch if we're in a transaction. To make sure
// we don't leak those settings, in the case of a transaction we first set up a save point which we can
// we don't leak those settings, in the case of a transaction, we first set up a save point which we can
// later roll back (taking the settings changes with it but NOT the lock). Because we can't confidently
// roll back a save point without knowing that it has been set up, we start the save point in its own
// query before we try-catch
var needsSavePoint = await HasTransactionAsync(connection).ConfigureAwait(false);
// query before we try-catch.
var needsSavePoint = await ShouldDefineSavePoint(connection).ConfigureAwait(false);

if (needsSavePoint)
{
using var setSavePointCommand = connection.CreateCommand();
Expand Down Expand Up @@ -124,9 +125,7 @@ private PostgresAdvisoryLock(bool isShared)
async ValueTask RollBackTransactionTimeoutVariablesIfNeededAsync(bool acquired)
{
if (needsSavePoint
// For transaction scoped locks, we can't roll back the save point on success because that will roll
// back our hold on the lock. It's ok to "leak" the savepoint in that case because it's an internally-owned
// transaction/connection and the savepoint will be cleaned up with the disposal of the transaction.
// For transaction scoped locks, we can't roll back the save point on success because that will roll back our hold on the lock.
&& !(acquired && UseTransactionScopedLock(connection)))
{
// attempt to clear the timeout variables we set
Expand Down Expand Up @@ -182,13 +181,17 @@ private DatabaseCommand CreateAcquireCommand(DatabaseConnection connection, Post
return command;
}

private static async ValueTask<bool> HasTransactionAsync(DatabaseConnection connection)
private static async ValueTask<bool> ShouldDefineSavePoint(DatabaseConnection connection)
{
if (connection.HasTransaction) { return true; }
if (!connection.IsExernallyOwned) { return false; }
// If the connection is internally-owned, we only define a save point if a transaction has been opened.
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.

@madelson madelson Jan 4, 2025 •
edited
Loading

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Tzachi009 Tzachi009 Jan 7, 2025 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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; }.

@madelson madelson Jan 8, 2025

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Tzachi009 Tzachi009 Jan 9, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (connection.HasTransaction) { return false; }

// If the connection is externally owned, then it might be part of a transaction that we can't
// see. In that case, the only real way to detect it is to begin a new one
// The externally-owned connection might still be part of a transaction that we can't see.
// In that case, the only real way to detect it is to begin a new one.
try
{
await connection.BeginTransactionAsync().ConfigureAwait(false);
Expand All @@ -199,6 +202,7 @@ private static async ValueTask<bool> HasTransactionAsync(DatabaseConnection conn
}

await connection.DisposeTransactionAsync().ConfigureAwait(false);

return false;
}

Expand All @@ -207,7 +211,13 @@ public ValueTask ReleaseAsync(DatabaseConnection connection, string resourceName

private async ValueTask ReleaseAsync(DatabaseConnection connection, PostgresAdvisoryLockKey key, bool isTry)
{
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.

@madelson madelson Jul 22, 2024

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Tzachi009 Tzachi009 Jul 23, 2024

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Tzachi009 Tzachi009 Aug 18, 2024

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Tzachi009 Tzachi009 Sep 26, 2024

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@madelson madelson Dec 3, 2024

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Tzachi009 Tzachi009 Dec 18, 2024

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (UseTransactionScopedLock(connection))
{
return;
}

using var command = connection.CreateCommand();
command.SetCommandText($"SELECT pg_catalog.pg_advisory_unlock{(this._isShared ? "_shared" : string.Empty)}({AddKeyParametersAndGetKeyArguments(command, key)})");
Expand Down Expand Up @@ -235,10 +245,9 @@ private static string AddKeyParametersAndGetKeyArguments(DatabaseCommand command
}

private static bool UseTransactionScopedLock(DatabaseConnection connection) =>
// This implementation (similar to what we do for SQL Server) is based on the fact that we only create transactions on
// internally-owned connections when doing transaction-scoped locking, and we only support transaction-scoped locking on
// internally-owned connections (since there's no explicit release).
!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;

@madelson madelson Jan 4, 2025

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Tzachi009 Tzachi009 Jan 7, 2025 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


private static string AddPGLocksFilterParametersAndGetFilterExpression(DatabaseCommand command, PostgresAdvisoryLockKey key)
{
Expand Down
17 changes: 17 additions & 0 deletions src/DistributedLock.Postgres/PostgresDistributedLock.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,17 @@ public PostgresDistributedLock(PostgresAdvisoryLockKey key, IDbConnection connec
{
}

/// <summary>
/// Constructs a lock with the given <paramref name="key"/> (effectively the lock name) and <paramref name="transaction"/>.
///
/// The provided <paramref name="transaction"/> will be used to connect to the database and will provide lock scope. It is assumed to be externally managed and
/// will not be committed or rolled back.
/// </summary>
public PostgresDistributedLock(PostgresAdvisoryLockKey key, IDbTransaction transaction)
: this(key, CreateInternalLock(key, transaction))
{
}

#if NET7_0_OR_GREATER
/// <summary>
/// Constructs a lock with the given <paramref name="key"/> (effectively the lock name) and <paramref name="dbDataSource"/>,
Expand Down Expand Up @@ -78,6 +89,12 @@ internal static IDbDistributedLock CreateInternalLock(PostgresAdvisoryLockKey ke
return new DedicatedConnectionOrTransactionDbDistributedLock(key.ToString(), () => new PostgresDatabaseConnection(connection));
}

internal static IDbDistributedLock CreateInternalLock(PostgresAdvisoryLockKey key, IDbTransaction transaction)
{
if (transaction == null) { throw new ArgumentNullException(nameof(transaction)); }
return new DedicatedConnectionOrTransactionDbDistributedLock(key.ToString(), () => new PostgresDatabaseConnection(transaction));
}

#if NET7_0_OR_GREATER
internal static IDbDistributedLock CreateInternalLock(PostgresAdvisoryLockKey key, DbDataSource dbDataSource, Action<PostgresConnectionOptionsBuilder>? options)
{
Expand Down
11 changes: 11 additions & 0 deletions src/DistributedLock.Postgres/PostgresDistributedReaderWriterLock.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,17 @@ public PostgresDistributedReaderWriterLock(PostgresAdvisoryLockKey key, IDbConne
{
}

/// <summary>
/// Constructs a lock with the given <paramref name="key"/> (effectively the lock name) and <paramref name="transaction"/>.
///
/// The provided <paramref name="transaction"/> will be used to connect to the database and will provide lock scope. It is assumed to be externally managed and
/// will not be committed or rolled back.
/// </summary>
public PostgresDistributedReaderWriterLock(PostgresAdvisoryLockKey key, IDbTransaction transaction)
: this(key, PostgresDistributedLock.CreateInternalLock(key, transaction))
{
}

#if NET7_0_OR_GREATER
/// <summary>
/// Constructs a lock with the given <paramref name="key"/> (effectively the lock name) and <paramref name="dbDataSource"/>,
Expand Down
14 changes: 14 additions & 0 deletions src/DistributedLock.Postgres/PostgresDistributedSynchronizationProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,20 @@ public PostgresDistributedSynchronizationProvider(IDbConnection connection)
this._readerWriterLockFactory = key => new PostgresDistributedReaderWriterLock(key, connection);
}

/// <summary>
/// Constructs a provider which connects to Postgres using the provided <paramref name="transaction"/>.
///
/// The provided <paramref name="transaction"/> will be used to connect to the database and will provide lock scope. It is assumed to be externally managed and
/// will not be committed or rolled back.
/// </summary>
public PostgresDistributedSynchronizationProvider(IDbTransaction transaction)
{
if (transaction == null) { throw new ArgumentNullException(nameof(transaction)); }

this._lockFactory = key => new PostgresDistributedLock(key, transaction);
this._readerWriterLockFactory = key => new PostgresDistributedReaderWriterLock(key, transaction);
}

#if NET7_0_OR_GREATER
/// <summary>
/// Constructs a provider which connects to Postgres using the provided <paramref name="dbDataSource"/> and <paramref name="options"/>.
Expand Down
3 changes: 3 additions & 0 deletions src/DistributedLock.Postgres/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Medallion.Threading.Postgres.PostgresDistributedLock.PostgresDistributedLock(Medallion.Threading.Postgres.PostgresAdvisoryLockKey key, System.Data.IDbTransaction! transaction) -> void
Medallion.Threading.Postgres.PostgresDistributedReaderWriterLock.PostgresDistributedReaderWriterLock(Medallion.Threading.Postgres.PostgresAdvisoryLockKey key, System.Data.IDbTransaction! transaction) -> void
Medallion.Threading.Postgres.PostgresDistributedSynchronizationProvider.PostgresDistributedSynchronizationProvider(System.Data.IDbTransaction! transaction) -> void