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

Keepalives support #999

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Keepalives support #999

wants to merge 4 commits into from

Conversation

marselester
Copy link

@marselester marselester commented Sep 30, 2020

Proposing to add keepalives and keepalives_interval parameters support #360.

By default KeepAlive is set to 15 seconds https://godoc.org/net#Dialer.

type Dialer struct {
    ...
    // KeepAlive specifies the interval between keep-alive
    // probes for an active network connection.
    // If zero, keep-alive probes are sent with a default value
    // (currently 15 seconds), if supported by the protocol and operating
    // system. Network protocols or operating systems that do
    // not support keep-alives ignore this field.
    // If negative, keep-alive probes are disabled.
    KeepAlive time.Duration
    ...
}

From what I understand, on Linux that would set

  • TCP_KEEPIDLE=15 (idle time) the time (in seconds) the connection needs to remain idle before TCP starts sending keepalive probes.
  • TCP_KEEPINTVL=15 (retry interval) the time (in seconds) between individual keepalive probes.

If TCP_KEEPCNT=8 (the maximum number of keepalive probes TCP should send before dropping the connection), then
the total timeout would be TCP_KEEPIDLE + TCP_KEEPINTVL * TCP_KEEPCNT = 15 seconds + 15 seconds * 8 pings = 135 seconds.

@marselester
Copy link
Author

Go 1.13 tests fail because I used if !errors.Is(err, tc.want) { ... } 🤔

FAIL: TestKeepaliveError/keepalives_interval_float (0.00s)

connector_test.go:133: expected: invalid syntax, got: invalid value for parameter keepalives_interval: strconv.ParseInt: parsing "1.1": invalid syntax

FAIL: TestKeepaliveError/keepalives_interval_whitespace (0.00s)

connector_test.go:133: expected: invalid syntax, got: invalid value for parameter keepalives_interval: strconv.ParseInt: parsing " ": invalid syntax

@alok87
Copy link

alok87 commented Sep 30, 2020

I am running the go process in a debian 8 OS. The connections does not get auto cleaned for me until i restart the go process. I am assuming it was because of bad connections not getting terminated.

By default 15 seconds do you mean Keep-alives in linux system with lib/pq are already working? And this PR makes it configurable?

@marselester
Copy link
Author

By default 15 seconds do you mean Keep-alives in linux system with lib/pq are already working?

They should be working golang/go#23459. You can check the number of probes with sysctl net.ipv4.tcp_keepalive_probes.

@alok87
Copy link

alok87 commented Sep 30, 2020

Its 9 for me, let me check why the connections are not getting cleaned.

$ sudo sysctl net.ipv4.tcp_keepalive_probes
net.ipv4.tcp_keepalive_probes = 9

@alok87
Copy link

alok87 commented Sep 30, 2020

Confirmed mine was a connection leak, db.Stats() helped a great deal !

sql.Open will use driver's OpenConnector instead of its
Open method. This will enable TCP keepalive support.

Note, testDriver was added for TestRuntimeParameters since it
expects old fashioned Driver interface (without OpenConnector).
@marselester
Copy link
Author

I haven't tried these changes on prod, but at least keepalives_interval=5 works in a simple demo https://github.com/marselester/pg-keepalive, i.e., sql.Open("postgres", dsn) and sql.OpenDB(connector).

setsockopt(3, SOL_SOCKET, SO_KEEPALIVE, [1], 4) = 0
setsockopt(3, SOL_TCP, TCP_KEEPINTVL, [5], 4) = 0
setsockopt(3, SOL_TCP, TCP_KEEPIDLE, [5], 4) = 0

Got rid of OpenConnector for simplicity's sake.

Noticed "pq: current transaction is aborted" in go18_test.go:212,
but it doesn't seem to be related. The same issue popped up in
lib#921.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants