I've removed the documentation around max_open/max_idle. I agree they could have been confusing and added a ping check at the start of each to make sure a working connection is available.

Just briefly looked over this, will include in 1.4. Let me preface this by saying I haven't used the sql package before. Doesn't the sql package reopen a new connection automatically if one is closed cleanly? What cases would we need to retry or wait until the next Gather loop?

Just briefly looked over this, will include in 1.4. Let me preface this by saying I haven't used the sql package before. Doesn't the sql package reopen a new connection automatically if one is closed cleanly? What cases would we need to retry or wait until the next Gather loop?

It does not. It passes the error to the client and lets the client handle it. This is because the connection might have been closed for a number of reasons that the client might need to handle, or log (so that admins are aware their connections are unexpectedly closing).

Pretty much any time we get a connection error we would need to immediately retry. Waiting until the next gather would not be a good idea for a few reasons.

1. If the connection was killed due to something like an idle timeout. Every other gather interval would fail. First gather runs, the pool is empty and gets a new connection, runs fine, and puts the connection in the pool. Some idle timer somewhere, which is shorter than gather interval kills the connection. Second gather runs, gets the connection from the pool, connection is dead, gather fails. Rinse & repeat.
2. The gather interval might be really infrequent. Such a reason might be is if the query is really expensive, you don't want to run it very often. So if gather interval is 15 minutes, that means you have a half hour between successful gathers.

Well that makes things tricky, wish it were more like http keepalive. Now I'm being really lazy, but does Ping() make a network call or just toss closed connections?

Well that makes things tricky, wish it were more like http keepalive. Now I'm being really lazy, but does Ping() make a network call or just toss closed connections?

Network call.

So in that case we shouldn't use it to ensure a connection.

You couldn't even if you wanted to. Aside from issues like it introduces latency in the operation, and the remote chance that something can happen between ping and query, the big issue is that there's no guarantee that the ping will happen on the same connection as the query.
The ping will check a connection out of the pool, do the ping, and put it back. Your query will check a connection out of the pool, do the query, and put it back. There's nothing that says your query will get the same connection as the ping.
The only way to ensure that 2 operations happen on the same connection is to use a transaction. And that just brings new problems you have to be aware of.

Doesn't the sql package reopen a new connection automatically if one is closed cleanly? What cases would we need to retry or wait until the next Gather loop?

To answer your question yes, on a clean close it'll happily create a new connection. on a bad close that complicates matters. which the ping will resolve by making the network call (if we actually care, I don't I only added it because phemmer seemed concerned about things that in reality are not a huge deal or are easily solved by maxlifetime/tcp keepalives). alot of phemmers comments are in my opinion making mountains out of mole hills.

here is the code for getting a connection:

// conn returns a newly-opened or cached *driverConn.

func (db *DB) conn(ctx context.Context, strategy connReuseStrategy) (*driverConn, error) {

	db.mu.Lock()

	if db.closed {

		db.mu.Unlock()

		return nil, errDBClosed

	}

	// Check if the context is expired.

	select {

	default:

	case <-ctx.Done():

		db.mu.Unlock()

		return nil, ctx.Err()

	}

	lifetime := db.maxLifetime


	// Prefer a free connection, if possible.

	numFree := len(db.freeConn)

	if strategy == cachedOrNewConn && numFree > 0 {

		conn := db.freeConn[0]

		copy(db.freeConn, db.freeConn[1:])

		db.freeConn = db.freeConn[:numFree-1]

		conn.inUse = true

		db.mu.Unlock()

		if conn.expired(lifetime) {

			conn.Close()

			return nil, driver.ErrBadConn

		}

		return conn, nil

	}


	// Out of free connections or we were asked not to use one. If we're not

	// allowed to open any more connections, make a request and wait.

	if db.maxOpen > 0 && db.numOpen >= db.maxOpen {

		// Make the connRequest channel. It's buffered so that the

		// connectionOpener doesn't block while waiting for the req to be read.

		req := make(chan connRequest, 1)

		reqKey := db.nextRequestKeyLocked()

		db.connRequests[reqKey] = req

		db.mu.Unlock()


		// Timeout the connection request with the context.

		select {

		case <-ctx.Done():

			// Remove the connection request and ensure no value has been sent

			// on it after removing.

			db.mu.Lock()

			delete(db.connRequests, reqKey)

			db.mu.Unlock()

			select {

			default:

			case ret, ok := <-req:

				if ok {

					db.putConn(ret.conn, ret.err)

				}

			}

			return nil, ctx.Err()

		case ret, ok := <-req:

			if !ok {

				return nil, errDBClosed

			}

			if ret.err == nil && ret.conn.expired(lifetime) {

				ret.conn.Close()

				return nil, driver.ErrBadConn

			}

			return ret.conn, ret.err

		}

	}


	db.numOpen++ // optimistically

	db.mu.Unlock()

	ci, err := db.driver.Open(db.dsn)

	if err != nil {

		db.mu.Lock()

		db.numOpen-- // correct for earlier optimism

		db.maybeOpenNewConnections()

		db.mu.Unlock()

		return nil, err

	}

	db.mu.Lock()

	dc := &driverConn{

		db:        db,

		createdAt: nowFunc(),

		ci:        ci,

	}

	db.addDepLocked(dc, dc)

	dc.inUse = true

	db.mu.Unlock()

	return dc, nil
}

Okay so lets go over the changes (ignoring ping I don't really care for that in general anyways)

this PR changes the code from:

func (p Postgres) Gather(...) {
db, err := sql.Open(...)
defer db.Close()
// ... execute queries
}

to:

func (p Postgres) Start(...) {
p.db, err := sql.Open(...)
// ...
}

func (p Postgres) Gather(...) {
  // ... execute queries
}

func (p Postgres) Shutdown() {
p.db.Close()
}

When maxlifetime is set to < the collection interval these two are functionally equivalent. if a system runs into an issue they'd run into it in either one. All this bullshit about idle network closing by infrastructure are resolved by TCP keepalives (which is by default 10minutes in the driver currently being used), except for the idle connection shutdowns coming directly from postgresql (this is the original source of invoking Ping as it would likely detect this), these are configuration issue on the end user's side we can do nothing about, but again is simply resolved by setting the maxlifetime to an appropriate value if necessary. However given I believe each of the methods on DB have a builtin retry with a fallback to a new connection I'm fairly confident it won't need any tuning, see the code snippet below for QueryContext.

If there is some idle connection termination in place on the user of telegraf's system that is their responsibility to configure it correctly. These are really not valid reasons to put a retry handling logic here. They are better handled in the drivers.

We are talking about a worst case scenario that in order for a end user to get the previous behaviour from these changes is that they have to configure maxlifetime to < than collection interval. On the plus side this will generally allow for longer lasting connections to the DB due to the TCP keepalives by the driver, and the fact we are not forcibly reconnecting everytime unless it is configured to do so.

If the connection was killed due to something like an idle timeout. Every other gather interval would fail. First gather runs, the pool is empty and gets a new connection, runs fine, and puts the connection in the pool. Some idle timer somewhere, which is shorter than gather interval kills the connection. Second gather runs, gets the connection from the pool, connection is dead, gather fails. Rinse & repeat.

the code begs to differ:

func (db *DB) QueryContext(ctx context.Context, query string, args ...interface{}) (*Rows, error) {

	var rows *Rows

	var err error

	for i := 0; i < maxBadConnRetries; i++ {

		rows, err = db.query(ctx, query, args, cachedOrNewConn)

		if err != driver.ErrBadConn {

			break

		}

	}

	if err == driver.ErrBadConn {

		return db.query(ctx, query, args, alwaysNewConn)

	}

	return rows, err

}

// putConn adds a connection to the db's free pool.

// err is optionally the last error that occurred on this connection.

func (db *DB) putConn(dc *driverConn, err error) {

	db.mu.Lock()

	if !dc.inUse {

		if debugGetPut {

			fmt.Printf("putConn(%v) DUPLICATE was: %s\n\nPREVIOUS was: %s", dc, stack(), db.lastPut[dc])

		}

		panic("sql: connection returned that was never out")

	}

	if debugGetPut {

		db.lastPut[dc] = stack()

	}

	dc.inUse = false


	for _, fn := range dc.onPut {

		fn()

	}

	dc.onPut = nil


	if err == driver.ErrBadConn {

		// Don't reuse bad connections.

		// Since the conn is considered bad and is being discarded, treat it

		// as closed. Don't decrement the open count here, finalClose will

		// take care of that.

		db.maybeOpenNewConnections()

		db.mu.Unlock()

		dc.Close()

		return

	}

	if putConnHook != nil {

		putConnHook(db, dc)

	}

	added := db.putConnDBLocked(dc, nil)

	db.mu.Unlock()


	if !added {

		dc.Close()

	}

}

So long as we don't skip a gather because postgres closed an idle connection we are good.

I'll run some tests this weekend to verify idle connection termination won't impact the gather stage.

To answer your question yes, on a clean close it'll happily create a new connection. on a bad close that complicates matters.

No it will not. The only "clean close" is if the client closes the connection, in which case it will not be in the pool, and is irrelevant to the issue at hand.

Obviously I'm unable to convince you, so here's some proof. I set up our firewall to drop idle connections after 15 seconds. I then configured telegraf to gather at 30 second intervals:

[agent]
  flush_interval = "1s"

[[outputs.file]]
  files = ["stdout"]
  tagexclude = ["host"]

[[inputs.postgresql_extensible]]
  interval = "30s"
  address = "host=172.28.128.7 sslmode=disable user=postgres dbname=postgres"
  tagexclude = ["db","server"]
  [[inputs.postgresql_extensible.query]]
    sqlquery = "select pg_backend_pid() as pid"

Here's the result of running telegraf with your PR:

# ./telegraf -config telegraf.conf
2017-04-26T03:56:25Z I! Starting Telegraf (version )
2017-04-26T03:56:25Z I! Loaded outputs: file
2017-04-26T03:56:25Z I! Loaded inputs: inputs.postgresql_extensible
2017-04-26T03:56:25Z I! Tags enabled: host=fll1000539l.local
2017-04-26T03:56:25Z I! Agent Config: Interval:10s, Quiet:false, Hostname:"fll1000539l.local", Flush Interval:1s 
postgresql pid=27953i 1493178990000000000
2017-04-26T03:57:00Z E! Error in plugin [inputs.postgresql_extensible]: read tcp 192.168.102.79:65339->172.28.128.7:5432: read: connection reset by peer
postgresql pid=28002i 1493179050000000000
2017-04-26T03:58:00Z E! Error in plugin [inputs.postgresql_extensible]: read tcp 192.168.102.79:65367->172.28.128.7:5432: read: connection reset by peer
postgresql pid=28056i 1493179110000000000
2017-04-26T03:59:00Z E! Error in plugin [inputs.postgresql_extensible]: read tcp 192.168.102.79:65396->172.28.128.7:5432: read: connection reset by peer

Notice how it's behaving EXACTLY as I predicted it would. Every other gather fails.

Please don't just dismiss my comments out of hand. I'm quite familiar with go & the sql library.

I'm happy to adjust my telegraf config with whatever settings you would like me to test, but I guarantee, nothing will fix it until a retry is put in the code.

@phemmer: and you didn't do the one thing I said would restore this code to the current behaviour in master: set max_lifetime to be less than your collection period.

Here is an example:
telegraf-lifetime-misconfigured.conf

[agent]
	flush_interval = "1s"
	
[[outputs.file]]
	files = ["stdout"]
	tagexclude = ["host"]

[[inputs.postgresql_extensible]]
	interval = "10s"
	# max_lifetime = "9s" this line is commented out to cause the error to occur as in your example.
	address = "host=127.0.0.1 user=postgres sslmode=disable application_name=telegraf port=5432"
	databases = ["postgres"]
	[[inputs.postgresql_extensible.query]]
		sqlquery = "select pg_backend_pid() as pid"

tcpkill -i lo port 5432

./bin/telegraf --config telegraf-lifetime-misconfigured.conf

2017-04-26T22:56:17Z I! Starting Telegraf (version dev-148-gee2b0828)
2017-04-26T22:56:17Z I! Loaded outputs: file
2017-04-26T22:56:17Z I! Loaded inputs: inputs.postgresql_extensible
2017-04-26T22:56:17Z I! Tags enabled: host=jambli
2017-04-26T22:56:17Z I! Agent Config: Interval:10s, Quiet:false, Hostname:"jambli", Flush Interval:1s 
postgresql,server=host\=127.0.0.1\ user\=postgres\ application_name\=telegraf\ port\=5432,db=postgres pid=4232i 1493247380000000000
2017-04-26T22:56:30Z E! Error in plugin [inputs.postgresql_extensible]: write tcp 127.0.0.1:46760->127.0.0.1:5432: write: connection reset by peer
postgresql,server=host\=127.0.0.1\ user\=postgres\ application_name\=telegraf\ port\=5432,db=postgres pid=4238i 1493247400000000000
2017-04-26T22:56:50Z E! Error in plugin [inputs.postgresql_extensible]: write tcp 127.0.0.1:46802->127.0.0.1:5432: write: connection reset by peer
postgresql,server=host\=127.0.0.1\ user\=postgres\ application_name\=telegraf\ port\=5432,db=postgres pid=4243i 1493247420000000000
2017-04-26T22:57:10Z E! Error in plugin [inputs.postgresql_extensible]: write tcp 127.0.0.1:46844->127.0.0.1:5432: write: connection reset by peer
postgresql,server=host\=127.0.0.1\ user\=postgres\ application_name\=telegraf\ port\=5432,db=postgres pid=4250i 1493247440000000000
2017-04-26T22:57:30Z E! Error in plugin [inputs.postgresql_extensible]: write tcp 127.0.0.1:46884->127.0.0.1:5432: write: connection reset by peer
2017-04-26T22:57:30Z I! Hang on, flushing any cached metrics before shutdown

notice it reproduces your example exactly. now watch what happens if I add in the max_lifetime (which causes the plugin to act the same as the code in master):
telegraf-lifetime-correctly-configured.conf

[agent]
	flush_interval = "1s"
	
[[outputs.file]]
	files = ["stdout"]
	tagexclude = ["host"]

[[inputs.postgresql_extensible]]
	interval = "10s"
	max_lifetime = "9s"
	address = "host=127.0.0.1 user=postgres sslmode=disable application_name=telegraf port=5432"
	databases = ["postgres"]
	[[inputs.postgresql_extensible.query]]
		sqlquery = "select pg_backend_pid() as pid"

tcpkill -i lo port 5432

./bin/telegraf --config telegraf-lifetime-correctly-configured.conf

2017-04-26T23:05:42Z I! Starting Telegraf (version dev-148-gee2b0828)
2017-04-26T23:05:42Z I! Loaded outputs: file
2017-04-26T23:05:42Z I! Loaded inputs: inputs.postgresql_extensible
2017-04-26T23:05:42Z I! Tags enabled: host=jambli
2017-04-26T23:05:42Z I! Agent Config: Interval:10s, Quiet:false, Hostname:"jambli", Flush Interval:1s 
postgresql,server=host\=127.0.0.1\ user\=postgres\ application_name\=telegraf\ port\=5432,db=postgres pid=4635i 1493247950000000000
postgresql,server=host\=127.0.0.1\ user\=postgres\ application_name\=telegraf\ port\=5432,db=postgres pid=4638i 1493247960000000000
postgresql,server=host\=127.0.0.1\ user\=postgres\ application_name\=telegraf\ port\=5432,db=postgres pid=4642i 1493247970000000000
postgresql,db=postgres,server=host\=127.0.0.1\ user\=postgres\ application_name\=telegraf\ port\=5432 pid=4647i 1493247980000000000
postgresql,server=host\=127.0.0.1\ user\=postgres\ application_name\=telegraf\ port\=5432,db=postgres pid=4650i 1493247990000000000
postgresql,server=host\=127.0.0.1\ user\=postgres\ application_name\=telegraf\ port\=5432,db=postgres pid=4653i 1493248000000000000
postgresql,server=host\=127.0.0.1\ user\=postgres\ application_name\=telegraf\ port\=5432,db=postgres pid=4656i 1493248010000000000
postgresql,server=host\=127.0.0.1\ user\=postgres\ application_name\=telegraf\ port\=5432,db=postgres pid=4668i 1493248020000000000
postgresql,server=host\=127.0.0.1\ user\=postgres\ application_name\=telegraf\ port\=5432,db=postgres pid=4672i 1493248030000000000
postgresql,server=host\=127.0.0.1\ user\=postgres\ application_name\=telegraf\ port\=5432,db=postgres pid=4676i 1493248040000000000
postgresql,server=host\=127.0.0.1\ user\=postgres\ application_name\=telegraf\ port\=5432,db=postgres pid=4680i 1493248050000000000
postgresql,server=host\=127.0.0.1\ user\=postgres\ application_name\=telegraf\ port\=5432,db=postgres pid=4684i 1493248060000000000
2017-04-26T23:07:42Z I! Hang on, flushing any cached metrics before shutdown

notice how its not erroring anymore.

long story short: at worst you'll get no benefits from this PR, you'll just need to adjust your configuration, you would have needed to adjust your configuration regardless if your firewall has a short termination period. but as I've said:

• retries are not the solution.
• there is an easy way to revert behaviour if your network has a idle connection killer that is less than 5 minutes.

I'm happy to do a follow up PR once this is in to expose the TCP keepalive setting so you can then get the full benefits of a persistant connection.

Please don't just dismiss my comments out of hand. I'm quite familiar with go & the sql library.

I never did, I went and double/triple checked database/sql and the pgx driver for possible issues.

The fact of the matter is if TCP Keepalives, or Max lifetime didn't solve the issue there would be something seriously wrong in either the driver or sql package.

Why should I have to add a config param to fix an issue this PR introduces? Changes should be non-breaking.
What if I'm a sys admin with a working config, and all of a sudden our network team decides to put in a idle timer on the firewalls. Telegraf is going to start breaking until I notice it and push out a config fix.

If max_lifetime is your only proposed way of restoring the previous behavior (non-persistence), this basically wastes a connection slot on the server, as the max lifetime is enforced when the connection is checked out from the pool.

retries are not the solution.

Why such an aversion to retries? A retry would fix the issue without any action from the user. It would also handle a plethora of other issues, such as random network interruption, db going down for maintenance, etc.

I imagine it is possible for the DB to configure the idle timeout on connections, perhaps via pgbouncer? I feel like the client shouldn't show any error if an idle connection is closed by the server. If it is set less than interval that would happen each gather, is this correct?

(My premises that idle connections should be allowed to be closed without effect is mostly based on my experience with http keep alive)

Also, if we do retries it should really just be a retry. No exponential backoffs or anything :)

Why should I have to add a config param to fix an issue this PR introduces?

• because its not an issue or a bug as I've shown. (its only an issue to you)
• because it will be clearly documented in the changelist.
• because telegraf has plenty examples of changes between new versions that require configuration changes.
• because its unrealistic to expect seamless upgrades between versions.
• because you only need to make the configuration change if your network has an aggressive idle tcp connection killing mechanism, i.e. its kills connections idling for less than 5 minutes.

If max_lifetime is your only proposed way of restoring the previous behavior (non-persistence), this basically wastes a connection slot on the server.

Yup, but in reality if you actually care about always being able to collect metrics you're going to reserve the slot anyways so this is a non-issue. If you don't care about always being able to collect metrics then why are you continuously pushing on retries? =)

Why such an aversion to retries? A retry would fix the issue without any action from the user.

because they make the code harder to maintain, its more code that needs to be debugged, they tend burn cpu cycles/time attempting code that will never succeed except in extremely rare cases (like random and very short network interruptions), and most importantly are just not needed in this case.

random network interruption

yup, but current master is also impacted by these, so nothing new would be introduced. so don't apply it to this pr. and even if it was a separate PR I'd argue they have a negative impact on actually collecting metrics each cycle. It also only helps with extremely short network interruptions, like milliseconds-seconds.

db going down for maintenance

if the db goes down for maintenance you can't collect metrics anyways, and guess what? now you just caused the gather to take much longer to complete (and likely still fail by the end anyways). again this is a non-issue.

I'm really tired of this. =)

I imagine it is possible for the DB to configure the idle timeout on connections, perhaps via pgbouncer?

pgbouncer does offer this, client_idle_timeout they obviously mark it as dangerous. =) I also tried using pgbouncer to reproduce this and it didn't work so I had to switch to tcpkill.

I feel like the client shouldn't show any error if an idle connection is closed by the server.

if its closed properly by the server it shouldn't. if its hard killed it might, but that would only happen similar to the firewall demonstration above. It shouldn't happen once gather actually starts.

If it is set less than interval that would happen each gather, is this correct?

if by 'it' you mean max_lifetime, and by 'that' create a new connection, then yes. =)

if by 'it' you mean max_lifetime, and by 'that' create a new connection, then yes. =)

What about if client_idle_timeout on pgbouncer is less than interval?

What about if client_idle_timeout on pgbouncer is less than interval?

that is effectively the same as my examples using tcpkill above. but like I said I couldn't get pgbouncer to terminate my idle connections between a 10s interval. I set client_idle_timeout to 1s and it kept my client connection around between cycles.

I should also mention the docs for this setting do encourage setting it properly:

Client connections idling longer than this many seconds are closed. This should be larger than the client-side connection lifetime settings, and only used for network problems.

If the connection is closed cleanly it should not affect subsequent gathers. If the firewall switches to DROP or someone digs up the cable with a backhoe then I'm okay with missing the next gather.

I'll fix this up again this weekend.

just an aside: the connection termination issue that was a hot topic in this PR can be addressed in a future PR. I managed to get changes necessary into database/sql to allow drivers to be easier to configure with connection details, such as a keepalive configuration. just waiting for it to land in go1.10, I'll make a PR to the driver and telegraf once that happens.

@james-lawrence Sorry to leave this one sitting for so long, is this ready for review?

@danielnelson its all good, yes its ready as far as I am concerned. there are some further improvements I can make once I finish getting changes upstream.