+1, better documentation would be good. We've had a lot of complexity added to this code over the years.

As far as I can tell,

• getConnection: Basic "connect to a DB" functionality.
  • Of note is the fact that if you pass a non-false $domain you're supposed to call reuseConnection() before you let it go out of scope, which is easy to forget.
• getServerConnection: The differences from getConnection seem fairly well documented. The code in LoadBalancer clearly matches.
• getAnyOpenConnection: Not very well documented. At the least, this one won't open a new connection (returning null if no connection is already opened), which can be useful if you know you can skip some operation if nothing else already connected to the DB. But there may be other differences as well, the implementation seems completely separate from getConnection/getServerConnection.
  • If the only difference is actually the "no new connection" behavior, ideally the implementation should reflect that by passing a flag to some internal method shared with getConnection or getServerConnection.
  • If there are other differences, which seems likely, and those differences shouldn't be fixed, I suspect this shouldn't be on the interface at all.
    • The three external callers only use it to get a DB_MASTER for locking, so perhaps something that returns a proxy that only exposes the locking-related methods would serve to reduce confusion.
• getConnectionRef: Wraps the handle returned by getConnection with a proxy object that will automatically call reuseConnection() when it goes out of scope.
• getLazyConnectionRef: Like getConnectionRef but additionally won't even call getConnection until the first time it's actually used. Good if you think it often won't wind up being used after all.
• getMaintenanceConnectionRef: Like getConnectionRef but with the proxy implementing IMaintainableDatabase rather than just IDatabase.

In T236880#5620706, @WDoranWMF wrote:

I think the next is we need input from @aaron from there we can schedule the docs update

Tagging our team as such.

In T236880#5620053, @Anomie wrote:

+1, better documentation would be good. We've had a lot of complexity added to this code over the years.

As far as I can tell,

• getConnection: Basic "connect to a DB" functionality.
  • Of note is the fact that if you pass a non-false $domain you're supposed to call reuseConnection() before you let it go out of scope, which is easy to forget.
• getServerConnection: The differences from getConnection seem fairly well documented. The code in LoadBalancer clearly matches.
• getAnyOpenConnection: Not very well documented. At the least, this one won't open a new connection (returning null if no connection is already opened), which can be useful if you know you can skip some operation if nothing else already connected to the DB. But there may be other differences as well, the implementation seems completely separate from getConnection/getServerConnection.
  • If the only difference is actually the "no new connection" behavior, ideally the implementation should reflect that by passing a flag to some internal method shared with getConnection or getServerConnection.
  • If there are other differences, which seems likely, and those differences shouldn't be fixed, I suspect this shouldn't be on the interface at all.
    • The three external callers only use it to get a DB_MASTER for locking, so perhaps something that returns a proxy that only exposes the locking-related methods would serve to reduce confusion.
• getConnectionRef: Wraps the handle returned by getConnection with a proxy object that will automatically call reuseConnection() when it goes out of scope.
• getLazyConnectionRef: Like getConnectionRef but additionally won't even call getConnection until the first time it's actually used. Good if you think it often won't wind up being used after all.
• getMaintenanceConnectionRef: Like getConnectionRef but with the proxy implementing IMaintainableDatabase rather than just IDatabase.

The addition of mw.org documentation along those lines would seem OK to me.

I think there are some additional nuances to consider here:

• getConnectionRef() should be the default for all foreign wiki DB connections because the caller must call ILoadBalancer::reuseConnection() for those when they're finished with the connection, so using getConnectionRef() can be a saner way of managing this unless the caller explicitly wants to control the lifecycle of the foreign DB connections it opens.
• @aaron seems to be working on some improvements to DB connection pooling in https://gerrit.wikimedia.org/r/q/I540b08920997c5 for T226595, which, when complete, would probably mean that all callers should prefer getConnectionRef() for both local and foreign connections.

Change 598544 had a related patch set uploaded (by Addshore; owner: Hoo man):
[mediawiki/core@master] ILoadBalancer::getConnection: Clarify when to reuseConnection

https://gerrit.wikimedia.org/r/598544

Change 598544 merged by jenkins-bot:
[mediawiki/core@master] rdbms: Clarify ILoadBalancer::getConnection doc for when to call reuseConnection

https://gerrit.wikimedia.org/r/598544

@aaron It seems getLazyConnectionRef and getConnectionRef are quite similar, perhaps similar enough that we don't need both?

I understand the benefit of deferring the connection attempt. But, I don't understand the benefit of not deferring the connection attempt. From what I can tell, they both end up calling getConnection() the same way, and without any other overhead avoided or added. If that is correct, then it seems worth standardising on the behaviour of getLazyConnectionRef, make getConnectionRef match that, and deprecate/remove getLazyConnectionRef.

Moving to Inbox as it is currently under Doing without assignee.