Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-26302

Avoid requiring long-lived per-Database-instance connections

    XMLWordPrintable

    Details

      Description

      Discussions on slack raised a very valid concern about our usage of connections that may be expected to stay alive for long time (at least the duration of a PipelineTask's runQuantum), noting that SQLAlchemy's connection pooling functionality (which we currently turn off) might address this.

      The solution is not as simple as having our Database class hold onto an Engine object and make a new Connection every time it is called, however - many Registry operations involve multiple calls to Database, and because these are expected to be part of the same transaction, they must all be executed by the same Connection.

      One way to address this would be to split the Database interface into a pair of classes, one per-engine and one per-connection. We could probably make only the former an ABC, giving it protected virtual methods that take a Connection object as an argument. The per-connection object would be concrete and final, and just hold the connection state and public forwarders to those protected methods.

      I'll have to think harder about how best to provide access to a per-connection object from the per-engine object, and which of those two should get the transaction method; I think that's the big open question.

      Before we embark on this change, it might be good to try to verify (perhaps just by reading the docs) that using SQLAlchemy connection pooling actually does provide some insulation from broken connections, as opposed to just seeming like something that ought to provide some insulation. If we need to implement our own reconnection logic, I'd like to know that up front.

        Attachments

          Issue Links

            Activity

            Hide
            salnikov Andy Salnikov added a comment -

            Thanks for review and suggestions! There is no separate Connection in a Session because it has to be shared with transactions and all other methods that need a connection. Also Database._tempTables is used by both classes, so right now Session is just a "subset" of Database which helps managing special methods for temp table management. Cleaner separation of concern would indeed be to have a Connection managed entirely by a Session (and not using Engine to execute queries) but that means many of Database methods have to be moved to Session class. Maybe we really want that but it also can be done in a separate ticket.

            Show
            salnikov Andy Salnikov added a comment - Thanks for review and suggestions! There is no separate Connection in a Session because it has to be shared with transactions and all other methods that need a connection. Also Database._tempTables is used by both classes, so right now Session is just a "subset" of Database which helps managing special methods for temp table management. Cleaner separation of concern would indeed be to have a Connection managed entirely by a Session (and not using Engine to execute queries) but that means many of Database methods have to be moved to Session class. Maybe we really want that but it also can be done in a separate ticket.
            Hide
            salnikov Andy Salnikov added a comment -

            Jim Bosch, did you check ctrl_mpexec PR, I do not see an approval for it?

            Show
            salnikov Andy Salnikov added a comment - Jim Bosch , did you check ctrl_mpexec PR, I do not see an approval for it?
            Hide
            tjenness Tim Jenness added a comment -

            Andy Salnikov can you file that followup ticket?

            Show
            tjenness Tim Jenness added a comment - Andy Salnikov can you file that followup ticket?
            Hide
            jbosch Jim Bosch added a comment -

            ctrl_mpexec approved (with one "curiosity" question); just forgot to hit the button before.

            Show
            jbosch Jim Bosch added a comment - ctrl_mpexec approved (with one "curiosity" question); just forgot to hit the button before.
            Hide
            salnikov Andy Salnikov added a comment -

            Merged and done. Opened DM-29005 for follow up task.

            Show
            salnikov Andy Salnikov added a comment - Merged and done. Opened DM-29005 for follow up task.

              People

              Assignee:
              salnikov Andy Salnikov
              Reporter:
              jbosch Jim Bosch
              Reviewers:
              Jim Bosch
              Watchers:
              Andy Salnikov, Jim Bosch, Kian-Tat Lim, Tim Jenness
              Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  CI Builds

                  No builds found.