Subject: Designing a better connection pool for psycopg3

From: Daniele Varrazzo <[email-masked]>
Date: Mon, 18 Jan 2021 13:04:31 +0100

Hello,

I've been gathering a few ideas about the connection pool I would like
to provide with psycopg3. I would be happy if you would like to take a
look and give some feedback.

https://www.psycopg.org/articles/2021/01/17/pool-design/

Thank you very much,

-- Daniele

---

From: Karsten Hilbert <[email-masked]>
Date: Mon, 18 Jan 2021 14:13:21 +0100

> https://www.psycopg.org/articles/2021/01/17/pool-design/

I would strongly advise against making sys.exit() the default
for pool.terminate() unless I misunderstand something.

Karsten
--
GPG  40BE 5B0E C98E 1713 AFA6  5BC0 3BEA AC80 7D4F C89B

---

From: Daniele Varrazzo <[email-masked]>
Date: Mon, 18 Jan 2021 14:50:34 +0100

> for pool.terminate() unless I misunderstand something.

How would you terminate the program if a maintenance thread, not the
main one, thinks that the program is not in working state?

Thank you!

-- Daniele

---

From: Magnus Hagander <[email-masked]>
Date: Mon, 18 Jan 2021 15:05:10 +0100

> main one, thinks that the program is not in working state?

Why would it be OK for a maintenance thread to terminate the program
at all? And certainly by default?

Wouldn't the reasonable thing to do be to flag the pool as broken, and
then just stop trying. Then whenever the application makes an attempt
to use the pool, it can he thrown an exception saying that this
happened.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

---

From: Karsten Hilbert <[email-masked]>
Date: Mon, 18 Jan 2021 15:15:55 +0100

> for pool.terminate() unless I misunderstand something.

IMO it should fall back to stopping the pooling machinery and
trying to return new connections on request.

Karsten

---

From: Karsten Hilbert <[email-masked]>
Date: Mon, 18 Jan 2021 15:19:44 +0100

> main one, thinks that the program is not in working state?

To me it is not the business of a library to terminate its
user (eg an application) upon resource starvation. After all,
the app may be perfectly fine with not being able to talk to
the database. Only it knows what to do under such
circumstances.

If one wants to support such machinery, I would suggest a
callback into the application, set up by the application
code.

Karsten

---

From: Daniele Varrazzo <[email-masked]>
Date: Mon, 18 Jan 2021 15:29:34 +0100

> to use the pool, it can he thrown an exception saying that this

I'm trying to imagine what happens in a case such as a network
partition or reconfiguration, and the app server doesn't see the
database anymore. This node is arguably broken.

If the reconnection thread fails to obtain new connections, and the
ones currently in the pool are discarded as detected broken,
eventually the pool is depleted.

The requesting threads (e.g. web requests handlers) would try to
obtain a connection, time out after e.g. 30 seconds, and receive an
error. The error would result in a 500 for the user, probably a sentry
exception and log in a file, but the program would likely not die.

The program could remain in this condition for an arbitrary long time,
until someone notices by looking at the logs and scratches their head
to understand how to fix the problem.

If the program dies, its manager would try to restart it, insisting
until the configuration makes the database visible again. A service
down in a crash loop is more visible than a service up and running but
not serving.

Anyway I appreciate that the default of terminating a program is
probably too aggressive. So I would remove the `terminate()` function
and base implementation and leave a `connection_failed()` handler,
with a default no-op implementation, which people preferring their
program to terminate can subclass (with `sys.exit(1)` or whatever
termination strategy they find useful).

-- Daniele

---

From: Daniele Varrazzo <[email-masked]>
Date: Mon, 18 Jan 2021 15:31:44 +0100

> callback into the application, set up by the application

Thank you for the feedback, Karsten. Probably a customisable no-op
callback is a better default behaviour :)

-- Daniele

---

From: Magnus Hagander <[email-masked]>
Date: Mon, 18 Jan 2021 15:39:20 +0100

> database anymore. This node is arguably broken.

Only if this is the *only* thing it does.

It might still be able to reach other services on other nodes. Other
databases. Heck, even other database son the same node if it was a
config error.

> exception and log in a file, but the program would likely not die.

Yes, and thus it would continue to serve all requests that don't need
a database connection from this particular pool.

> to understand how to fix the problem.

The *program* can still decide to do this. By itself doing an exit(1)
when the exception is thrown (per my suggestion above), or by just not
catching that exception at all.

> down in a crash loop is more visible than a service up and running but

But what about the other services that potentially ran in the same process?

It's an extremely simplistic view to think that a single web server
will only ever talk to a single database, *and* require this database
for all it's operations. Yet that's what this default would do.

The entire point of a connection pool is persistent processes that do
more than one thing, after all. And there is (surprising maybe, but
there is) still an entire world out that that isn't just serving
individual web requests.

> termination strategy they find useful).

That would definitely work. A plugin-point for this would be very useful.

And if you want to avoid the "timeout based error" in the client,
adding a flag to the pool saying "this pool is currently unhealthy" in
this case would work, and then whenever someone tries to get a
connection out of this pool it can throw an exception immediately
instead of waiting for a timeout to happen.

---

From: Karsten Hilbert <[email-masked]>
Date: Mon, 18 Jan 2021 15:42:46 +0100

> termination strategy they find useful).

+1

Some apps may just gladly carry on buffering data locally,
and sending them to the backend next time they get a chance
to do so.

Karsten

---

From: Daniele Varrazzo <[email-masked]>
Date: Mon, 18 Jan 2021 15:43:54 +0100

> instead of waiting for a timeout to happen.

This is a good idea, thank you :)

-- Daniele

---

From: Karsten Hilbert <[email-masked]>
Date: Mon, 18 Jan 2021 15:44:15 +0100

> instead of waiting for a timeout to happen.

+1 :-)

Karsten

---

From: Daniele Varrazzo <[email-masked]>
Date: Mon, 18 Jan 2021 16:12:05 +0100

> databases. Heck, even other database son the same node if it was a

In your opinion (and Karsten's, if he'd like to chip in), what should
happen on program start? Is attempting to create a new connection in
the main thread, and throwing an exception if failing to do so, a
reasonable behaviour?

Cheers,

-- Daniele

---

From: Karsten Hilbert <[email-masked]>
Date: Mon, 18 Jan 2021 16:19:49 +0100

> the main thread, and throwing an exception if failing to do so, a

Sure ! The app can always catch that and decide what to do.

Karsten

---

From: Magnus Hagander <[email-masked]>
Date: Mon, 18 Jan 2021 16:20:18 +0100

> the main thread, and throwing an exception if failing to do so, a

Yes, I think that's perfectly reasonable.

The application is always going to have to be ready to get an
exception on the call to get a connection out of the pool. If it cares
about the "early startup failure", it should just try to grab a
connection immediately on startup and will then notice if it fails.

---

From: Vladimir Ryabtsev <[email-masked]>
Date: Mon, 18 Jan 2021 10:20:12 -0800

> Connection configuration
In my opinion it is not only static vs dynamic,
I have a feeling that subclassing gives more freedom for improper
implementation (resulting in problems). Also it depends on internal
API (not necessarily public), which puts more obligations on the
maintainer to keep it compatible with old releases. In this case
I like the configuring approach better
(and it does not exclude subclassing as well).

> Exclusive connections?
The feature itself seems useful, but what are the benefits of getting it
from the pool (e.g. checking state, reconnection, configuration)
as opposed to creating one manually?
Is such a connection counted in regard to 'minconn', 'maxconn'?

> What to do in case of connection failure?
> A first step towards a sane behaviour could be to die early, on startup,
if the connection is not working when the first pool population is done
Agree.
BTW, at which moment will the driver be trying to connect? At pool
initialization or on first "getconn()"?

> What if the database is missing in action during the program lifetime?
Following connection attempts may be repeated, with an exponential backoff,
until dying after a few minutes of fruitless attempts
Yes, I would prefer exponential backoff, with a timeout prior to "panic"
callback.
Preferably, both configurable.

> Connections usage pattern
I don't see cases where an application should bother about it,
choose whatever is better for your implementation. And yes, stack
looks reasonable.

> 'minconn', Proposed default: 4
Many services and applications do not really need many connections
but would like to benefit from automatic reconnections. So the default
is questionable, as many people will leave it unchanged.

> "get_info()"
One thing that comes into my head every time I use connection pools
is how many are really needed for my application. Usually it is adjusted
experimentally or using some performance tests. Can you collect and
provide some stats to get more insights on that? I see you want to
expose some in "get_info()", which exactly? E.g. number of times
the max limit was hit, current number of connections acquired?

> 'max_lifetime_sec'
Why is it necessary at all? Does a connection get rotten with time?
As I understand, you are planning to check a connection state in
background. As long as it is OK, what is the need to replace it?

> num_workers, default: 3
Assuming every worker does the same job, is there really the amount
of work for three workers?

"get_maintenance_task()"
This API seems to be too advanced, in what cases you see a sane use case
for it?

Thank you,
Vladimir

---

From: Karsten Hilbert <[email-masked]>
Date: Mon, 18 Jan 2021 19:45:27 +0100

> as opposed to creating one manually?

It might decrease the necessary import surface.

Karsten

---

From: Karsten Hilbert <[email-masked]>
Date: Mon, 18 Jan 2021 19:49:17 +0100

> is questionable, as many people will leave it unchanged.

I would, perhaps, default to 1 *per thread*. Or to 2 per
thread, one readonly, and one readwrite, if the application
cares (requests ro vs rw, that is). But that's just me and my
use case :-)

Karsten

---

From: Karsten Hilbert <[email-masked]>
Date: Mon, 18 Jan 2021 20:03:35 +0100

> initialization or on first "getconn()"?

I tend to favor the latter, as a default.

As you said, an application can always attempt to grab a
connection early on to detect failures.

Karsten