sfackler / rust-postgres

Native PostgreSQL driver for the Rust programming language
Apache License 2.0
3.46k stars 439 forks source link

Support timeouts for COPY commands #1109

Open seanlinsley opened 7 months ago

seanlinsley commented 7 months ago

COPY doesn't respect statement_timeout, which for us has led to stalled connections on production. This can also be confirmed locally:

$ time psql -c "set statement_timeout = '1s'; select pg_sleep(2);"
SET
ERROR:  canceling statement due to statement timeout

real    0m1.028s
user    0m0.020s
sys 0m0.005s

$ time psql -c "set statement_timeout = '1s'; copy foo from stdin" < /dev/zero
SET
^CERROR:  COPY from stdin failed: canceled by user
CONTEXT:  COPY foo, line 1

real    0m4.816s
user    0m1.781s
sys 0m3.031s

Another project has noticed this issue and worked around it: https://github.com/npgsql/npgsql/issues/1328 https://github.com/npgsql/npgsql/commit/9fda64a3f4fe6964af3d9931aa8c9c038c4eb2fa

Here's the relevant configuration from our application (note we're also using deadpool for connection pooling):

let mut pg_config = tokio_postgres::Config::from_str(url).unwrap();
pg_config.options("-c timezone=UTC -c statement_timeout=120s");
pg_config.connect_timeout(Duration::from_secs(30));
pg_config.keepalives_idle(Duration::from_secs(5 * 60));

I see there is tcp_user_timeout which could possibly help, but it's apparently not straightforward to use because of surprising behavior: https://blog.cloudflare.com/when-tcp-sockets-refuse-to-die

sfackler commented 7 months ago

You can just use tokio's own timeout functionality: https://docs.rs/tokio/latest/tokio/time/fn.timeout.html

seanlinsley commented 7 months ago

In this case the COPY command survived across an application deploy (where the server is shut down and completely replaced), so a tokio timeout wouldn't have helped.

sfackler commented 7 months ago

Can you explain to me how the linked npgsql change would have helped there? If the client process is no longer running, there's nothing it can do to affect the state of the database.

seanlinsley commented 7 months ago

I don't understand the npgsql change myself (and the PR doesn't go into detail), I just linked to it because it's the only place I've found where others have discussed this issue.

Upstream Postgres should definitely document this, and if possible enforce statement_timeout on COPY commands. If there isn't a TCP setting (or other thing) that can help, we will likely revert back to using INSERT to avoid this issue.

sfackler commented 7 months ago

To be very clear, the npgsql change will not solve your problem. If the server is not detecting that its client has vanished, you need to change the behavior of the server, not the behavior of the client.

Is the server blocked indefinitely trying to read more data from the client? If that's the case, you should probably enable TCP keepalive probes on the server side (and look into why your server shutdown process isn't cleanly terminating connections). If the server's blocked something else internally, then you'd need to ask the Postgres maintainers to do something about that.