Skip to content

Do not retry queries if a transaction is open

Simon Tomlinson requested to merge load-balancer-transaction-leak into master

What does this MR do and why?

Describe in detail what your merge request does and why.

Disables retries in the load balancer if a transaction is open. Previously we retried the literal sql statement on a new connection, which lost the beginning of the transaction without the ruby process knowing. This change disables retries if in a transaction, since it is not safe to retry in this case.

Resolves DB load balancer: Automatic retries may leak qu... (#220242 - closed)

Should this be behind a feature flag?

This is a change to a delicate part of our load-balancing system, so in general I'd prefer to use a feature flag here. However, the code to check feature flags runs through this code path when querying feature flag state from the database, leading to infinite recursion if one is checked here. The code could be made significantly more complex to break this recursion dependency, but I believe the risks of adding a feature flag and the possible recursion there outweigh the risks of this relatively simple change.

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

The included specs validate the behavior, but if you'd like to do so normally, you could do the following (which is very similar to what the specs execute):

Open a psql console and a rails console;

In the rails console, execute the following

ApplicationRecord.transaction { ApplicationRecord.connection.execute('select pg_sleep(20)'); }

During the 20 seconds of sleep, run the following sql to terminate the sleeping connection (and all other connections to your local database):

SELECT pg_terminate_backend(pg_stat_activity.pid)
FROM pg_stat_activity
WHERE pg_stat_activity.datname = current_database() 
AND pid <> pg_backend_pid();

Observe different behavior based on this MR: Without this MR, the sleep will be retried. With this MR, an error will be raised.

Repeat the same procedure without the .transaction {} component, and see that the sleep is retried in both cases without a transaction open.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Simon Tomlinson

Merge request reports