Skip to content

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

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 5 commits into from
Jan 28, 2025
+221 −17
Merged

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

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cfb4f9a
Improve comments
Tzachi009 Jan 7, 2025
3010de2
Add support for capturing and restoring timeout settings for Postgres…
Tzachi009 Jan 20, 2025
dfb2cd2
Changes after CR
Tzachi009 Jan 22, 2025
afdc2d6
New test
Tzachi009 Jan 26, 2025
fe04d5e
Fixes after CR
Tzachi009 Jan 27, 2025
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
13 changes: 8 additions & 5 deletions src/DistributedLock.Postgres/PostgresAdvisoryLock.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,12 +186,14 @@ private static async ValueTask<bool> ShouldDefineSavePoint(DatabaseConnection co
// If the connection is internally-owned, we only define a save point if a transaction has been opened.

@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.

@Tzachi009 see https://github.com/madelson/DistributedLock/pull/222/files#r1906204137

@Tzachi009 Tzachi009 Jan 20, 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.

@madelson I added the functionality for capturing and restoring the timeout settings, please review it.

You should know that I tried to remove the save point functionality entirely, but turns out that you can't do that. In case you get an error when attempting to acquire the lock, you must roll back the transaction (or the save point) if you want to run any other DB command, otherwise you will get an error about the aborted transaction.

@madelson madelson Jan 22, 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.

In case you get an error when attempting to acquire the lock, you must roll back the transaction (or the save point) if you want to run any other DB command, otherwise you will get an error about the aborted transaction.

What exactly are the cases impacted by this? Cancellation? Timeout? Something else? I'm wondering because it seems like these would also be issues for the new APIs, e.g. someone passes in a cancellation token or does TryAcquire with a timeout and it silently dooms the transaction that feels like it would be pretty undesirable and unexpected, particularly in the former case. Thoughts?

@Tzachi009 Tzachi009 Jan 22, 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.

The transaction will not be doomed with the current code, because I didn't remove the save point functionality. They will be functional even if an exception will occur.

When I did try to remove it - it did impact the handling of the timeout (because it checks if the lock is held) and the new code that try to restore the previous timeout settings.

The RollBackTransactionTimeoutVariablesIfNeededAsync function still rolls back the save point for transactional locks in cases of any failure (when you get any exception - the lock is assumed as unobtained), and thus the transaction isn't aborted (see the if statement - if (needsSavePoint && !(acquired && UseTransactionScopedLock(connection)))). This was true even before my changes.

There is only one edge case that I can think of, where the save point roll back works against you in a way for transactional locks, but it probably existed before my changes (for internal connection's transactions) - when you get a timeout and still hold the lock because of the Postgres bug (lines 88-96), I assume the save point's roll back will release the lock beforehand, so you will get a timeout even though for a moment you acquired the lock every time, but this is an edge case that isn't really visible for the library's users.

@madelson madelson Jan 23, 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.

Sorry I wasn't clear. What I'm trying to understand is how this interacts with the new transactional locking APIs that do not get to use the savepoint.

Under what conditions does the transaction get doomed? Can this happen under "normal" operation of the lock?

Said another way, if the savepoint is critical for all the existing lock APIs, how is removing it not a problem for the new APIs?

@Tzachi009 Tzachi009 Jan 23, 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.

I am actually slightly more confused by your questions now :).

I think that maybe you didn't notice, but I changed the code so the new transactional locking APIs will always use save points, in addition to the capture/restore settings logic (see line 227) - this is because otherwise the transaction will get doomed when you get any exception while trying to acquire the lock.

So, the bottom line - the transaction will never get doomed in the new APIs, since we keep using save points in this scenario.
Just wanted to let you know that save points can't be removed from the code, because otherwise transactions will indeed reach an aborted state.

@madelson madelson Jan 24, 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.

the new transactional locking APIs will always use save points

Ok sorry didn't realize this. However, now I'm wondering why it is OK to "leak" a SAVEPOINT on every lock acquisition. I'm not an expert on Postgres transactions, but this seems like a side-effect that could cause issues for people. At minimum, subsequent lock acquisitions on the same connection would try to recreate the same named SAVEPOINT which probably wouldn't work (would be good to add a test case for subsequent lock acquisitions on the same transaction).

I wonder if the real solution here is to restrict the new APIs wrt timeout/cancellation functionality. If we only support TryAcquire with timeout 0 and no cancellation, then we dodge all the issues with setting leaks and savepoint leaks (we can modify the code so that for 0-timeout acquires it doesn't issue any SET LOCAL commands and therefore also doesn't need the SAVEPOINT. Thoughts?

@Tzachi009 Tzachi009 Jan 26, 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.

Your point regarding a "nested" save point with the same name was really good, so I wrote a test and checked it, and it seems to work as intended. I wondered why that is, so I went and read postgres' documentation (https://www.postgresql.org/docs/current/sql-savepoint.html):

savepoint_name
The name to give to the new savepoint. If savepoints with the same name already exist, they will be inaccessible until newer identically-named savepoints are released.

SQL requires a savepoint to be destroyed automatically when another savepoint with the same name is established. In PostgreSQL, the old savepoint is kept, though only the more recent one will be used when rolling back or releasing. (Releasing the newer savepoint with RELEASE SAVEPOINT will cause the older one to again become accessible to ROLLBACK TO SAVEPOINT and RELEASE SAVEPOINT.) Otherwise, SAVEPOINT is fully SQL conforming.

If you have any other concern regarding this scenario, please do tell.

As for the timeout/cancellation restrictions in the new API as a solution for the leaked save point - I am against it. It will make the new API less useful but more importantly it won't solve the issue. We can't really dodge it, if you get any exception from the DB, not a timeout or a cancellation specifically, the transaction will be dead without a save point roll back, so the restrictions won't be a safe option to prevent this scenario from happening altogether.

I think that bottom line is that the save point must be leaked in this case, because if you roll it back after an acquisition you don't have a lock, and if you don't use a save point then the transaction may be aborted (which is the least favorable option IMO), so I'm not seeing a way around it.
We can add a note about it in the new API methods, so users will be able to choose whether they can/should use the API.

@madelson madelson Jan 26, 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.

Good to know that leaking a savepoint does not cause a conflict with further acquisitions!

I do want to confirm that there isn't any other observable side-effect to leaking savepoints that could cause issues for users. I'm not finding anything from my quick reading, but curious if you know either way. I want to avoid a scenario where calling the API has some hidden side-effect that causes issues for users, leading to bug reports here.

@Tzachi009 Tzachi009 Jan 27, 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.

I am not aware of any other issue that may occur because of that. Do you think we should add a note about it in the new API?

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.
// If the connection is externally-owned with an established transaction,
// it means that the connection came through the transactional locking APIs (see PostgresDistributedLock.Extensions class),
// and we can't define a save point since it can't be released in case the lock will be acquired (the lock will be released too in this scneario).
if (connection.HasTransaction) { return false; }

// 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.
// This can only be the case if the externally-owned connection didn't came through the transactional locking APIs (see PostgresDistributedLock.Extensions class).
// In that case, the only real way to detect the transaction is to begin a new one.
try
{
await connection.BeginTransactionAsync().ConfigureAwait(false);
Expand All @@ -203,6 +205,7 @@ private static async ValueTask<bool> ShouldDefineSavePoint(DatabaseConnection co

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

// If we reached this point, it means the externally-owned connection has no transaction, therefore we can't define a save point.
return false;
}

Expand Down Expand Up @@ -239,8 +242,8 @@ private static string AddKeyParametersAndGetKeyArguments(DatabaseCommand command
}

private static bool UseTransactionScopedLock(DatabaseConnection connection) =>
// Transaction-scoped locking is supported on both externally-owned and internally-owned connections,
// as long as the connection has a transaction.
// Transaction-scoped locking is supported on internally-owned connections and externally-owned connections which explicitly have a transaction
// (meaning that the external connection came through the transactional locking APIs, see PostgresDistributedLock.Extensions class).
connection.HasTransaction;

private static string AddPGLocksFilterParametersAndGetFilterExpression(DatabaseCommand command, PostgresAdvisoryLockKey key)
Expand Down