Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maintain min_pool_size for pools with forced user (even if no client conns) #947

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 13 additions & 0 deletions doc/config.md
Expand Up @@ -137,6 +137,12 @@ Add more server connections to pool if below this number.
Improves behavior when the normal load suddenly comes back after a period
of total inactivity. The value is effectively capped at the pool size.

Only enforced for pools where at least one of the following is true:

* the entry in the `[database]` section for the pool has a value set for the
`user` key (aka forced user)
* there is at least one client connected to the pool

Default: 0 (disabled)

### reserve_pool_size
Expand Down Expand Up @@ -1080,6 +1086,13 @@ the `default_pool_size` is used.
Set the minimum pool size for this database. If not set, the global `min_pool_size` is
used.

Only enforced if at least one of the following is true:

* this entry in the `[database]` section has a value set for the `user` key
(aka forced user)
* there is at least one client connected to the pool


### reserve_pool

Set additional connections for this database. If not set, `reserve_pool_size` is
Expand Down
1 change: 1 addition & 0 deletions include/server.h
Expand Up @@ -21,6 +21,7 @@ void kill_pool_logins(PgPool *pool, const char *sqlstate, const char *msg);
int pool_pool_mode(PgPool *pool) _MUSTCHECK;
int pool_pool_size(PgPool *pool) _MUSTCHECK;
int pool_min_pool_size(PgPool *pool) _MUSTCHECK;
int database_min_pool_size(PgDatabase *db) _MUSTCHECK;
int pool_res_pool_size(PgPool *pool) _MUSTCHECK;
int database_max_connections(PgDatabase *db) _MUSTCHECK;
int user_max_connections(PgUser *user) _MUSTCHECK;
21 changes: 20 additions & 1 deletion src/janitor.c
Expand Up @@ -515,7 +515,7 @@ static void check_pool_size(PgPool *pool)
cur < pool_pool_size(pool) &&
cf_pause_mode == P_NONE &&
cf_reboot == 0 &&
pool_client_count(pool) > 0) {
(pool_client_count(pool) > 0 || pool->db->forced_user != NULL)) {
log_debug("launching new connection to satisfy min_pool_size");
launch_new_connection(pool, /* evict_if_needed= */ false);
}
Expand Down Expand Up @@ -680,6 +680,25 @@ static void do_full_maint(evutil_socket_t sock, short flags, void *arg)
if (cf_pause_mode == P_SUSPEND)
return;

/*
* Creating new pools to enable `min_pool_size` enforcement even if
* there are no active clients.
*
* If clients never connect there won't be a pool to maintain the
* min_pool_size on, which means we have to proactively create a pool,
* so that it can be maintained.
*
* We are doing this for databases with forced users only, to reduce
* the risk of creating connections in unexpected ways, where there are
* many users.
_ */
statlist_for_each_safe(item, &database_list, tmp) {
db = container_of(item, PgDatabase, head);
if (database_min_pool_size(db) > 0 && db->forced_user != NULL) {
get_pool(db, db->forced_user);
}
}

statlist_for_each_safe(item, &pool_list, tmp) {
pool = container_of(item, PgPool, head);
if (pool->db->admin)
Expand Down
11 changes: 9 additions & 2 deletions src/server.c
Expand Up @@ -210,12 +210,19 @@ int pool_pool_size(PgPool *pool)
return pool->db->pool_size;
}

/* min_pool_size of the pool's db */
int pool_min_pool_size(PgPool *pool)
{
if (pool->db->min_pool_size < 0)
return database_min_pool_size(pool->db);
}

/* min_pool_size of the db */
int database_min_pool_size(PgDatabase *db)
{
if (db->min_pool_size < 0)
return cf_min_pool_size;
else
return pool->db->min_pool_size;
return db->min_pool_size;
}

int pool_res_pool_size(PgPool *pool)
Expand Down
2 changes: 2 additions & 0 deletions test/test.ini
Expand Up @@ -2,6 +2,8 @@
p0 = port=6666 host=127.0.0.1 dbname=p0 user=bouncer pool_size=2
p0x= port=6666 host=127.0.0.1 dbname=p0 min_pool_size=5 pool_size=5
p0y= port=6666 host=127.0.0.1 dbname=p0 min_pool_size=5 pool_size=5 max_db_connections=2
; for testing 'min_pool_size' with forced user
;p0z= port=6666 host=127.0.0.1 dbname=p0 min_pool_size=3 user=pswcheck
p1 = port=6666 host=127.0.0.1 dbname=p1 user=bouncer
p2 = port=6666 host=127.0.0.1 dbname=p0 max_db_connections=4
p3 = port=6666 host=127.0.0.1 dbname=p0 user=bouncer pool_mode=session
Expand Down
36 changes: 31 additions & 5 deletions test/test_limits.py
@@ -1,4 +1,5 @@
import asyncio
import re

import psycopg
import pytest
Expand Down Expand Up @@ -37,10 +38,35 @@ async def test_pool_size(pg, bouncer):

@pytest.mark.asyncio
async def test_min_pool_size(pg, bouncer):
bouncer.admin("set min_pool_size = 3")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test has been changed to not set the global min_pool_size anymore. Caused too many connection creations in a number of pools overloading the github test runner.
Rather this test check various combinations with individual pools.

assert pg.connection_count("p1") == 0
# uncommenting the db that has "forced" maintenance enabled
# by not having this db enabled we avoid polluting other tests
# with connections getting autocreated
with bouncer.ini_path.open() as f:
original = f.read()
with bouncer.ini_path.open("w") as f:
# uncomment the relevant db
new = re.sub(r"^;p0z= (.+)", r"p0z= \g<1>", original, flags=re.MULTILINE)
print(new)
f.write(new)
bouncer.admin("reload")

# having to wait a little to give janitor time to create connection to satisfy min_pool_size
await asyncio.sleep(2)

# It's a bit tricky to get the timing of this test to work
# ensure db without min_pool_size has no connections
# p0
assert pg.connection_count(dbname="p0", users=("bouncer",)) == 0
# ensure db with min_pool_size and forced user (p0z) has the required
# backend connections
assert pg.connection_count(dbname="p0", users=("pswcheck",)) == 3

# ensure db with min_pool_size and no forced user (p0x) has no backend
# connections
assert pg.connection_count(dbname="p0", users=("postgres",)) == 0
# client connecting to p0x should trigger backend connection creation up to
# min_pool_size.
#
# NOTE: It's a bit tricky to get the timing of this test to work
# robustly: Full maintenance runs three times a second, so we
# need to wait at least 1/3 seconds for it to notice for sure
# that the pool is in use. When it does, it will launch one
Expand All @@ -49,10 +75,10 @@ async def test_min_pool_size(pg, bouncer):
# Also, we need to keep the query running while this is
# happening so that the pool doesn't become momentarily
# unused.
result = bouncer.asleep(2, dbname="p1")
result = bouncer.asleep(2, dbname="p0x")
await asyncio.sleep(2)
await result
assert pg.connection_count("p1") == 3
assert pg.connection_count(dbname="p0", users=("postgres",)) == 5


def test_min_pool_size_with_lower_max_user_connections(bouncer):
Expand Down