Only run pg_advisory_unlock_all if necessary.

[moser]

When using PALs on a shared SQLAlchemy engine, running pg_advisory_unlock_all on every connection pool checkin event can cause performance problems.

This PR adds tracking for used DB API connections and will limit the pg_advisory_unlock_all to connections that were actually used by PALs.

[rsyring]

Thanks for the PR. Can you give me more context about how you are using pals with respect to your other database connections? I've always setup a separate dedicated connection pool for the locks. It sounds like you are sharing the pals connection pool with code that is doing more than locking?

[moser]

I use pals for locking in the context of depeche-db. The users of that library only supply a single SQLAlchemy engine which is then used to do both locking and other DB interactions. It is possible to configure the locking manually and use a separate connection pool, but it makes the usage of the library less comfortable.

But even if you exclusively use the connection pool for locking, it is still inefficient to run pg_advisory_unlock_all every time a connection is checked in to the pool. If the application fails to acquire a lock, you will end up with unnecessary unlock operations. I adapted the PR accordingly to only "taint" a connection if the lock was acquired successfully.

I understand that you are somewhat reluctant to accept changes to a safety measure. If you decline them, I will find another way to deal with it. For me it is important because the repeated calls take ~20-50ms each and waste hundreds of milliseconds on a critical path :-)

[rsyring]

That's a fine use case, thanks for sharing.

It seems like a rather innocuous change and I'm a +1 on changing it. However, I might want to see this ran under a "feature flag" at first so we can get some real-world usage data before making it the default.

Have you been able to run this modification in the real world yet (through a vendored version of this library or fork)?

I'd also want to see tests added for the functionality.

[moser]

@rsyring I tried a solution for registering the checkin handler only once but failed. There are just too many sqlalchemy specifics (across versions) involved to make it reliable.

Thus, I decided to continue on this solution. I added tests and put the tracking behavior behind a flag for now. WDYT?

[moser]

Simpler solution in #50