Skip to content

pipeline-manager: set the row lock timeout per transaction - #6865

Merged
ryzhyk merged 2 commits into
mainfrom
fix-manager-lock-timeout-pgbouncer
Aug 15, 2026
Merged

pipeline-manager: set the row lock timeout per transaction#6865
ryzhyk merged 2 commits into
mainfrom
fix-manager-lock-timeout-pgbouncer

Conversation

@ryzhyk

@ryzhyk ryzhyk commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

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.

Describe Manual Test Plan

Checklist

  • Unit tests added/updated
  • Integration tests added/updated
  • Documentation updated
  • Changelog updated

Breaking Changes?

Mark if you think the answer is yes for any of these components:

Describe Incompatible Changes

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>
@ryzhyk
ryzhyk requested a review from gz August 14, 2026 19:13
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>
@ryzhyk
ryzhyk force-pushed the fix-manager-lock-timeout-pgbouncer branch from e21ffb0 to 8ec0248 Compare August 14, 2026 19:56
@ryzhyk

ryzhyk commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

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?;

@gz gz Aug 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@ryzhyk ryzhyk Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@gz gz Aug 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you saying we wrap it in our own type with the same API

yes

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can do as a follow up

@ryzhyk
ryzhyk added this pull request to the merge queue Aug 14, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 15, 2026
@ryzhyk
ryzhyk added this pull request to the merge queue Aug 15, 2026
Merged via the queue into main with commit 061d708 Aug 15, 2026
1 check passed
@ryzhyk
ryzhyk deleted the fix-manager-lock-timeout-pgbouncer branch August 15, 2026 02:38
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