pipeline-manager: set the row lock timeout per transaction - #6865
Conversation
Every transaction now applies `lock_timeout` itself with `SET LOCAL`,
rather than the connection carrying it as an `options` startup parameter.
The startup parameter locks the manager out of any database reached
through PgBouncer, which rejects startup parameters it does not know:
FATAL: unsupported startup parameter: options
Neither the pool nor the separate connection that listens on the pipeline
table can be opened, so the manager cannot reach the database at all.
`SET LOCAL` holds for a direct connection and for every pooling mode: a
pooler keeps a transaction on one server connection from BEGIN to COMMIT,
and Postgres reverts the setting at commit, so it cannot leak to the next
client of that connection. It costs one round trip per transaction, and
`db::transaction` documents the trade-off.
`db::transaction::{begin, begin_read_only}` are now the only places that
start a transaction, which keeps a call site from ending up without the
timeout. The timeout itself is unchanged at 10 seconds.
One intended consequence: the transactions refinery runs for the
migrations no longer have a lock timeout. That is what a rolling upgrade
wants, as an instance starting up then waits for the migration of another
one to finish rather than failing to start.
Signed-off-by: Leonid Ryzhyk <ryzhyk@gmail.com>
A failing database call reported the category its failure fell into and
nothing else, so a connection that PgBouncer refuses read:
Error: Postgres connection pool error: 'Error occurred while
creating a new object: db error'
`tokio_postgres::Error` displays that category and leaves the message the
server sent to the error it wraps, which the report dropped. Appending the
innermost cause turns the same failure into:
Error: Postgres connection pool error: 'Error occurred while
creating a new object: db error: FATAL: unsupported startup
parameter in options: lock_timeout'
The Postgres, pool and migration errors now all report that cause, both in
`Display` and in the JSON the API returns. Only the two ends of the chain
are needed, as every wrapper in between displays the error it got.
`DBError` still exposes no `source`, so a caller that walks the chain
itself cannot repeat what the message already carries.
Signed-off-by: Leonid Ryzhyk <ryzhyk@gmail.com>
e21ffb0 to
8ec0248
Compare
|
I tested locally with pgbouncer. Created an issue in qa to add it to CI. |
| let default_tenant = TenantRecord::default(); | ||
| let mut client = self.pool.get().await?; | ||
| let txn = client.transaction().await?; | ||
| let txn = transaction::begin(&mut client).await?; |
There was a problem hiding this comment.
it seems very easy to forget/do wrong in the future by calling client.transaction() again, causing a regression(?)
Ideally, I was hoping for maybe the client overriding the .transaction method
another thought: how many queries really use/leverage this lock mechanism IIRC just one/a few? maybe they can just add the SET in the prepared stmt?
There was a problem hiding this comment.
Ideally, I was hoping for maybe the client overriding the .transaction method
Client is defined in another crate. Are you saying we wrap it in our own type with the same API?
There was a problem hiding this comment.
unrelated thoughts, how many queries really use/.everage this lock mechanism IIRC just one/afew? maybe they can just add the SET in the prepared stmt?
That fails ("ERROR: cannot insert multiple commands into a prepared statement").
Plus there are apparently 25 locations where it's needed.
And it's not clear that other database operations don't benefit from the timeout.
There was a problem hiding this comment.
Are you saying we wrap it in our own type with the same API
yes
Every transaction now applies
lock_timeoutitself withSET LOCAL, rather than the connection carrying it as anoptionsstartup parameter. The startup parameter locks the manager out of any database reached through PgBouncer, which rejects startup parameters it does not know:Neither the pool nor the separate connection that listens on the pipeline table can be opened, so the manager cannot reach the database at all.
SET LOCALholds for a direct connection and for every pooling mode: a pooler keeps a transaction on one server connection from BEGIN to COMMIT, and Postgres reverts the setting at commit, so it cannot leak to the next client of that connection. It costs one round trip per transaction, anddb::transactiondocuments the trade-off.db::transaction::{begin, begin_read_only}are now the only places that start a transaction, which keeps a call site from ending up without the timeout. The timeout itself is unchanged at 10 seconds.One intended consequence: the transactions refinery runs for the migrations no longer have a lock timeout. That is what a rolling upgrade wants, as an instance starting up then waits for the migration of another one to finish rather than failing to start.
Describe Manual Test Plan
Checklist
Breaking Changes?
Mark if you think the answer is yes for any of these components:
Describe Incompatible Changes