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

[#361] Add a max_connection_age setting #365

Merged
merged 1 commit into from
Apr 24, 2023

Conversation

hao-tian-xu
Copy link
Contributor

This defines how long a connection will live in seconds

  • Add a max_connection_age member to struct configuration. Its implementation and related functions are similar to idle_timeout
  • Add a start_time member to struct connection. Its implementation is similar to timestamp

@hao-tian-xu
Copy link
Contributor Author

hao-tian-xu commented Apr 3, 2023

Issue #361

  1. I've tried two ways for background checks. One is to add code to idle timeout functions, and another is to add separate functions. Both can work.
    • I currently prefer the separate method to have different time intervals of background checks.
    • If it's ok, I'll add new TRACKER and STATE, and a new Prometheus metric.
  2. A start_time member is added to struct connection as the start timestamp. It is initialized in pgagroal_return_connection if prefilled or in pgagroal_get_connection if not.
  3. I did a few simple tests. Any suggestions on how to do comprehensive testing?
  4. Another trivial question: should I set the default value to 0 to be consistent with idle_timeout?

src/libpgagroal/pool.c Outdated Show resolved Hide resolved
@jesperpedersen
Copy link
Collaborator

Otherwise it looks good

@hao-tian-xu
Copy link
Contributor Author

Thanks for the feedback! I added new STATE, TRACKER, Prometheus metric, and documentation for max_connection_age

@hao-tian-xu hao-tian-xu marked this pull request as ready for review April 5, 2023 11:53
@@ -60,7 +60,8 @@ The available keys and their accepted values are reported in the table below.
| log_connections | `off` | Bool | No | Log connects |
| log_disconnections | `off` | Bool | No | Log disconnects |
| blocking_timeout | 30 | Int | No | The number of seconds the process will be blocking for a connection (disable = 0) |
| idle_timeout | 0 | Int | No | The number of seconds a connection is been kept alive (disable = 0) |
| idle_timeout | 0 | Int | No | The number of seconds a connection is been kept alive since becomes idle (disable = 0) |
| max_connection_age | 0 | Int | No | The number of seconds a connection is been kept alive since starts (disable = 0) |
Copy link
Collaborator

Choose a reason for hiding this comment

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

"... since it times out"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I get it right,

  • idle_timeout is "The number of seconds a connection can remain idle before it is automatically closed"
  • max_connection_age is "The number of seconds a connection can remain active before it is automatically closed if it is not being used"

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is the maximum seconds that a connection will live - it will be checked upon returned to the pool, or during idle timeout

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback! I updated the max_connection_age docs and kept the idle_timeout docs untouched.

Checking upon returned to the pool is added to pgagroal_return_connection.

Both ways of checking are tested using a basic setup.

The number of seconds a connection is been kept alive since becomes idle (disable = 0). Default is 0

max_connection_age
The number of seconds a connection is been kept alive since starts (disable = 0). Default is 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto


config = (struct configuration*)shmem;

/* Kill the connection, if it lives longer than max_connection_age */
now = time(NULL);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need some extra checks here, right ?

Is max_connection_age > 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review! I've tested thses cases this time:

  • idle_timeout off, max_connection_age on, one prefill, no connection / get one connection and then close
  • idle_timeout on, max_connection_age off, one prefill, get one connection and then close
  • idle_timeout = 20, max_connection_age = 40, one prefill, get two connections and then close

Copy link
Collaborator

Choose a reason for hiding this comment

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

Excellent

@jesperpedersen
Copy link
Collaborator

Can you rebase ?

This defines how long a connection will live in seconds
- Add a `max_connection_age` member to `struct configuration`. It will be checked upon returned to the pool, or during idle timeout.
- Add new STATE, TRACKER, and Prometheus metric for `max_connection_age`
- Add documentation for `max_connection_age`
- Add a `start_time` member to `struct connection`. Its implementation is similar to `timestamp`
@hao-tian-xu
Copy link
Contributor Author

Sure, rebased and fixed conflicts

@jesperpedersen jesperpedersen merged commit 6112ec6 into agroal:master Apr 24, 2023
@jesperpedersen
Copy link
Collaborator

Merged.

Thanks for your contribution !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants