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

[Tzachi009]

Hi, I noticed that since version 1.1.0 of DistributedLock.Postgres, the library supports transaction-scoped advisory locks, if you choose to use them. I understand that it was added as part of the following issue - #168, as it turned out that using prepared statements/session-scoped advisory locks with PgBouncer is problematic.

The problem is that we must use PgBouncer, but in our case the app uses the library by passing an already established DB connection with a transaction, in order to have a better integrity of the lock. When I looked into the code of the library, I realized that the transaction locks are not supported for external connections:

I wonder why that is.
Is the issue that it will be the user's responsibilty to explicitly release the lock by commiting/rollbacking the transaction?
Will it be possible to add support for such scenarios?

Thanks.

[madelson]

@Tzachi009 thanks for your interest in the library. IIRC, the issue is that Postgres transaction-scoped locks can’t be released without ending the transaction. So either we’re killing the users transaction for them or releasing the lock doesn’t work properly. I don’t like either of those options from an API perspective.

are you hoping to run queries on the same transaction that holds the lock or is it more just the convenience of injecting the connection in your setup?

[Tzachi009]

Thank you for the quick response @madelson.

Yes, it is about running queries on the same transaction that holds the lock, and being able to use PgBouncer in such a case. In the critical path of the app, I acquire locks and then invoke several commands in Postgres (in a transaction) under the locks. Performance is very important in the critical path, so using the same transaction that holds the lock, instead of creating a new one seems reasonable. Moreover, this solution improves the integrity of the lock by using the same DB connection and transaction.

I understand your point regarding the API perspective, but if the API already allows the user to pass an external connection that is not under its control, then why not let the user who created the transaction have the responsibilty to end it?

Maybe another option could be to let the user configure whether the library or the user will be the responsible party of the transaction (via the PostgresConnectionOptionsBuilder class or when passing the DB connection to the API).

[Tzachi009]

Hi @madelson, may we continue discussing the suggestion or are you entirely opposed to the idea?

My team and I would really like to see this feature implemented, and I don't mind contributing to the library myself.

[madelson]

@Tzachi009 I think I can get behind the idea that in Postgres if you create a lock with an explicit transaction and then release the lock, it disposes the transaction. It's a little weird, but less weird than having release just noop. The user will be responsible for releasing the lock after committing the transaction.

Given those caveats, I do think it is worth thinking about what you are really getting from the library in this case vs. just issuing the same SQL commands yourself.

I'd be happy to have to you take a crack at this. Two implementation challenges I foresee:

1. We'll need to figure out a plan for test coverage, since the main suite that most locks run against probably won't work under the "one time use" constraint described above
2. PostgresAdvisoryLock.cs today makes use of save points and session variables to control the lock command; not sure how well any of that will behave with the added constraint of an external transation. I do think it is vital that we not pollute the transaction with overrides to session variables.

[Tzachi009]

Thank you, I will start to look into it soon and will see how it goes.

Just a few points regarding what you said:

• The transactional advisory locks are released automatically when the transaction is commited or rollbacked (https://www.postgresql.org/docs/current/explicit-locking.html#ADVISORY-LOCKS), therefore that's the only the user will have to do in order to release the lock, while the library won't be able to do so.
• As a user, I can still see the advatange of using the library in such a case, since it still consolidates everything regarding the management of the lock.
• I need to get more familiar with the tests, but I will check how to test this specific scenario. I will also try to check what we can do regarding save points/session variables.

[madelson]

@Tzachi009 another avenue I was considering is just offering this as a static method that acquires the lock on an existing transaction, returning no result. That would make it clear that it is the users responsibility to release.

[Tzachi009]

It may be a little weird to have a specific method just for this scenario, but that's a viable option, although I still think that when you send an external connection to the library, it is pretty clear that it is the user's responsibility to manage it, including a transaction, if one exists, therefore for a start I think we should still try to make it work with the existing API.

You said that the static method won't return a result, but what would you expect to happen if the lock can't be acquired? Should such API offer both Acquire() and TryAcquire() methods? Only for Postgres?

[madelson]

If we do the static method, it would return a Boolean result for TryAcquire and no result for Acquire.

What appeals to me about the static method is that it avoids implementing the IDistributedLock interfaces in a way that doesn’t really conform to the way all other implementations behave. That would add complexity and overhead for maintaining the library itself but also for users who likely wouldn’t know the ins and outs of Postgres locking and might be surprised by the library’s behavior.

[Tzachi009]

Hi @madelson,

I opened a PR with small initial changes - #222.
I would be glad to receive feedback from you, to know that I am on the right track.

I will summarize the current changes and some of the missing changes in the PR, which we may need to discuss/approve:

1. I decided to add a method that let you pass an external IDbTransaction. I understand that the static methods appeal to you, but I noticed that in the SQLServer/MySql API, the library let you pass an external transaction and the user is warned about the expected behavior in the method's summary, so I think the methods I added do conform to other implementations.
2. My initial suggestion was that the user will use the already existing method with an external IDbConnection, but with an established transaction, so the library will prefer to use a transaction scoped lock, but as described in point no.1, I changed my opinion - I think having a specific method to pass the transaction will make both the library's behavior and the user's responsibilty clearer.
   However, assuming that you will approve of the new method, now there is another question - is it ok for the method with the external IDbConnection to keep behave like it does today? Because currently, whether you pass an external connection with or without a transaction, a session scoped advisory lock will be acquired regardless, and I don't think it is obvious for the user (it sure wasn't for me, until I looked into the code).
   Do you think it should keep working like this? Or add support for my initial suggestion where a connection with a transaction will acquire a transaction scoped lock? Or perhaps we should check if the external connection has a transaction, and if so, throw a NotSupportedException with a message that tells the user to use the new method? Both options 2 and 3 are technically breaking changes of course, and I personally think option 3 is the best option, but it's your call.
3. Regarding the save point - I currently disabled it specifically for the case where the user pass an external transaction, since if the lock has been acquired, you can't release the save point anyway. I am not entirely sure if this is the right call, but I currently prefer it to be gone in this scenario, to minimize the external transaction pollution as you asked.
4. Regarding the timeouts session variables - in the case where the timeouts were already changed within the external transaction, I guess we may override their values and we should avoid it. I have a simple plan to store the current values of the timeouts before we change them (if a save point was not created), using the following query for example (see https://www.postgresql.org/docs/current/functions-admin.html#FUNCTIONS-ADMIN-SET):
   SELECT current_setting('lock_timeout', true)
   After we finished trying to acquire the lock, I will set the timeouts to the previous stored values. What do you think?
   And by the way, I don't think that we can set local variables in cases where a transaction wasn't initiated. Noticed that in such cases, Postgres logs a warning about it, so maybe I will add a check around that logic:
   
5. I didn't add tests yet, only tested/debugged manually. I want to wait for your approval regarding the changes before I'll add them.