Is it possible to disable the Ping that occurs when acquiring a connection in pgxpool?

[db-instacart]

pgx/pgxpool/pool.go

Lines 510 to 516 in b301530

	if res.IdleDuration() > time.Second {
	err := cr.conn.Ping(ctx)
	if err != nil {
	res.Destroy()
	continue
	}
	}

I see it was added for:

Efficiently check if a connection has been closed before writing.
This reduces the cases where the application doesn't know if a query
that does a INSERT/UPDATE/DELETE was actually sent to the server or
not.

Our use case is using pgx in read only proxies, so if this check is purely for INSERT/UPDATE/DELETE then it seems like we are doing extra pings that aren't needed.
Possibly more noticeable for us as we have many pools configured, so likely connections can go idle for > 1 second.
Perhaps some config driven way to disable it?

[jackc]

At the moment there is no way to configure or disable this.

I'm not opposed to changing this, but if something is to be done it should be more flexible than a simple switch. Probably a user supplied function that returns true or false to determine if the connection should be considered fresh or needs to ping.

[db-instacart]

Sure, is it something you see living on the Config in pool.go?

[jackc]

Sure, is it something you see living on the Config in pool.go?

Yes.

[db-instacart]

Hi Jack,

Was taking a look this morning at the code and had a question. What would you find useful to provide to the user to make the decision on whether or not they should do this liveness check? I'm not aware of everything that exists for use in the package, but right now it's using the IdleDuration from the puddle.Resource. Do you want to just provide the puddle.Resource similar to below?

// On the Config
CheckLiveness func(*puddle.Resource[*ConnResource]) bool

// Default func to use in the ParseConfig
func checkLiveness(res *puddle.Resource[*ConnResource]) bool {
	return res.IdleDuration() > time.Second
}

// And the snippet in Acquire
cr := res.Value()
if p.checkLiveness(res) {
    err := cr.conn.Ping(ctx)
    if err != nil {
        res.Destroy()
        continue
   }
}

This would involve making ConnResource public so it can be used in the function parameter.

Or do you want to pass something else to their user provided function?

[jackc]

Hmm... Good question. I'm not super eager to make ConnResource public. Though as you say, there isn't anything public that would give you the idle duration.

Maybe something like:

type ShouldPingParams struct {
	Conn *pgx.Conn
	IdleDuration time.Duration
}

func ShouldPing(ctx context.Context, params ShouldPingParams) bool {
	return params.IdleDuration > time.Second
}

Using a struct allows us to add new variables in the future if we forgot something. The context is valuable to smuggle in any values from the outside as well as cancellation if for some reason the check is doing something that could block.

[db-instacart]

Hi Jack,

Sorry for the delay. Any issue with allocations of ShouldPingParams in this hot path? Will each ShouldPingParams need to live around as long as the *pgx.Conn it contains also is still active?
If not I can go ahead and spin up a PR for it.

[jackc]

Since ShouldPingParams is passed by value I would not expect there to be any allocations at all.

[db-instacart]

Ah ha, for some reason I was thinking because the struct had a pointer perhaps it would cause an issue, if that's not the case though then great.

[ringerc]

This is also an issue for stdlib - there's no way to disable this pinging, and it's a problem for some proxies.

The stdlib code unconditionally calls Ping, and that's unconditionally propagated through all the layers to the underlying connection.

See related #272

Brought up in pgsty/pg_exporter#56

[ilyam8]

@db-instacart hey 👋 any plans on implementing this? Due to the current limitations, we're forced to use both v4 (PgBouncer) and v5 (Postgres) packages. I'd like to help, but I'm not sure I know enough about the project to do it well.

[DEliasVCruz]

Why not an interface?

Something like:

// On the Config
HealthCheck HealthChecker

// Default func to use in the ParseConfig
type HealthChecker interface {
	ShouldCheckHealth(context.Context) bool
	CheckHealth(context.Context, *pgx.Conn) error
}

type DefaultHealthChecker struct {
	IdleDuration time.Duration
}

func (hc DefaultHealthChecker) ShouldCheckHealth(context.Context) bool {
	return hc.IdleDuration > time.Second
}

// This so that users may iplement some custom way to check for the conection health
// Like how pg bouncer allows for sending a `do-nothing query` like `select 1`
func (hc DefaultHealthChecker) CheckHealth(ctx context.Context, con *pgx.Conn) error {
	return con.Ping(ctx)
}

// And the snippet in Acquire
if ShouldCheckHealth(ctx) {
    err := CheckHealth(ctx, cr.conn)
    if err != nil {
        res.Destroy()
        continue
   }
}

Sorry if I'm missing something, just throwing a quick idea based on previous comments and a quick skim of the code

[jackc]

@DEliasVCruz I'm not opposed to adding configuration to control how health is checked. But I think that should be a separate interface from deciding if health should be checked. It's quite possible someone would want to customize only one or the other and they shouldn't need to implement both.

[DEliasVCruz]

@jackc Understandable, then how about making it more granular? Asumming that there could be users who just want, the health check behaviour to be disable all together, having them implement a function whose only execution will be return false seems redundant

How about then having separate config options:

// On the Config
type Config struct {
	...
	EnableHealthCheck bool // Defaults to true
	HelthCheckConditionFunction func(ctx context.Context, map[string]any) bool
	HelthCheckConditionParams map[string]any
	HealthCheckQuery func(ctx context.Context, con *pgx.Conn) error
}

// Default function for Health Check Condition
func HelthCheckConditionFunction(ctx context.Context,  params map[string]any) bool {
	idleDuration, ok := params["idleDuration"].(time.Duration)
	if !ok {
		// I'm not sure what should be the expected behaviour in this case
		return false
	}

	return idleDuration > time.Second
}

// Default function for Health Check Query
func HealthCheckQuery(ctx context.Context, con *pgx.Conn) error {
	return con.Ping(ctx)
}

// And the snippet in Acquire
HelthCheckConditionParams["idleDuration"] = res.IdleDuration()

if EnableHealthCheck && HelthCheckConditionFunction(ctx, HelthCheckConditionParams) {
    err := HealthCheckQuery(ctx, cr.conn)
    if err != nil {
        res.Destroy()
        continue
   }
}

This way users who just want health check disabled can pass false to the config option and be done with it, and others can opt to modify the health check condition function/params or the query function or both

Using your idea of a struct for ShouldPingParams I think is simpler but also means that if users want more parameters, then that struct has to be continually updated. So why not use a simple map as a more flexible, although more verbose implementation, option? Aaaannd I'm not sure what should be expected behaviour if the cast of "idleDurationis notok` 🤔