Listen port list #1307
Listen port list #1307
Conversation
src/main.c
Outdated
| p = malloc(sizeof(cf_listen_port)); | ||
| safe_strcpy(p, cf_listen_port, sizeof(p)); |
There was a problem hiding this comment.
These sizeof calls are wrong. They should be using strlen. sizeof only works in this manner for static arrays (which these pointers are not).
There was a problem hiding this comment.
This is now fixed.
There was a problem hiding this comment.
This indeed seems quite useful. One future-looking thing would be to allow a database entry in the config to specify the transaction pooling port and the session pooling port. That way a single server connection pool could be used for both transaction and session pooling. This would probably be done best by introducing a new pool_mode called something like mixed.
src/pooler.c
Outdated
| int port; | ||
|
|
There was a problem hiding this comment.
let's move this declartion to the if block where it's actually used to limit its scope.
There was a problem hiding this comment.
This should be fixed now.
src/server.c
Outdated
| @@ -651,7 +651,7 @@ static bool handle_connect(PgSocket *server) | |||
| char buf[PGADDR_BUF + 32]; | |||
| bool is_unix = pga_is_unix(&server->remote_addr); | |||
|
|
|||
| fill_local_addr(server, sbuf_socket(&server->sbuf), is_unix); | |||
| fill_local_addr(server, sbuf_socket(&server->sbuf), is_unix, 0); | |||
There was a problem hiding this comment.
Why are we passing 0 here? Would be good to add a comment on fill_local_addr and fill_remote_addr in which cases 0 is expected.
There was a problem hiding this comment.
The zero here is definitely wrong. Need to figure out how to source the actual port given that with this commit we cant assume that the actual port is just whatever is set as the listen port. Looks like the is a pga_ function that is designed to get the port number from a PgSocket object.
It would probably have to be 3 ports: transaction, session, and statement. Currently the way I prefer to separate pool mode access is by user (is user_1_session, user_1_transaction, etc.). I understand the point that this is somewhat wasteful in terms of connection sharing though. The thing I don't like about using ports to separate pool mode access is there is no way to restrict access like we can with user (some classes of users are only allowed to use statement mode, etc). Also port numbers are inherently less meaningful to end users than database/usernames in the absence of service discovery. |
Another much more forward looking thing: would be very nice if the wire protocol allowed a pool_mode parameter to be sent to the server that pgbouncer could use to decide the pooling policy of the connection. Some adjacent discussion in this link. https://www.postgresql.org/message-id/flat/20250310230900.01b58a29%40ardentperf.com |
| db->port = cf_listen_port; | ||
| /* choose first port to assign to admin database */ | ||
| p = malloc(sizeof(cf_listen_port)); | ||
| safe_strcpy(p, cf_listen_port, sizeof(p)); |
There was a problem hiding this comment.
| safe_strcpy(p, cf_listen_port, sizeof(p)); | |
| safe_strcpy(p, cf_listen_port, strlen(cf_listen_port)); |
| /* fake database */ | ||
| db = add_database("pgbouncer"); | ||
| if (!db) | ||
| die("no memory for admin database"); | ||
|
|
||
| db->port = cf_listen_port; | ||
| /* choose first port to assign to admin database */ | ||
| p = malloc(sizeof(cf_listen_port)); |
There was a problem hiding this comment.
| p = malloc(sizeof(cf_listen_port)); | |
| p = malloc(strlen(cf_listen_port) + 1); |
One common use case of pgbouncer, in my experience is a proxy. There are many times where there are many applications with hardcoded non standard (5432 or 6432) port numbers and admins are forced to set up multiple pgbouncers and slowly find the offending apps and move people over. This is problematic in that it complicates the infrastructure, wastes resources, makes enforcing connection limits harder, etc.
Another use case is in providing a second unique port so that a particular pgbouncer instance can be accessed while the first port is shared with another instance for load balancing/zero downtime restarts.
This PR gives the ability for pgbouncer to listen to multiple ports.
One known outstanding issue with this PR: currently the port number displayed on the admin console is not correct, it just chooses the first port listed in the config.