Re: REPACK (CONCURRENTLY) decoding worker is canceled by lock_timeout
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t253770psql -h localhost -U postgresBuilt from patchset v3 (message #3), September 16, 2026 at 02:09 PM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t253770_3 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t253770_3 && git checkout t253770_3Patchset v3 (message #3) is on t253770_3
On Sep 12, 2026, at 00:33, Álvaro Herrera <alvherre@kurilemu.de> wrote:
On 2026-Sep-10, Chao Li wrote:
On Sep 10, 2026, at 11:21, shihao zhong <zhong950419@gmail.com> wrote:
Done in v2, through the DSM segment the worker already attaches to.
I think this is pretty reasonable.
Auto-vacuum explicitly overrides all four settable session timeouts
(statement_timeout, transaction_timeout, lock_timeout, and
idle_in_transaction_session_timeout) to zero, while this worker only
handles the latter two. I understand that statement_timeout and
idle_in_transaction_session_timeout are probably never armed by this
worker, so functionally they may not need special handling.Hmm, but REPACK is not autovacuum; it's quite different in fact, in that
REPACK is intended to always be invoked manually, while autovacuum runs
on its own. On the other hand, because REPACK refuses to run in a
transaction block, transaction_timeout and
idle_in_transaction_session_timeout don't really apply, so I'm not
seeing the potential for problems.
Yeah, I fully understood the difference. My concern was only about the inconsistency.
My concern is that the inconsistency might lead to confusion to future
readers. Does it make sense to either remove those two from
auto-vacuum worker or set them to repack worker as well?I decidedly don't want to touch autovacuum. Although I'm not sure I see
the reason why the transaction-based timeouts are relevant for
autovacuum.
That was actually my concern. The fact that this raised the question of why autovacuum resets those timeouts suggests that the inconsistency can be confusing to readers.
I agree we don't need to touch autovacuum in this patch. Does it make sense to remove those unnecessary timeout resets from autovacuum by a separate patch?
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Import Notes
Reply to msg id not found: aqQslOwhDkbNYerk@alvherre.pgsqlReference msg id not found: aqQslOwhDkbNYerk@alvherre.pgsql
On Sep 12, 2026, at 10:57, Chao Li <li.evan.chao@gmail.com> wrote:
On Sep 12, 2026, at 00:33, Álvaro Herrera <alvherre@kurilemu.de> wrote:
On 2026-Sep-10, Chao Li wrote:
On Sep 10, 2026, at 11:21, shihao zhong <zhong950419@gmail.com> wrote:
Done in v2, through the DSM segment the worker already attaches to.
I think this is pretty reasonable.
Auto-vacuum explicitly overrides all four settable session timeouts
(statement_timeout, transaction_timeout, lock_timeout, and
idle_in_transaction_session_timeout) to zero, while this worker only
handles the latter two. I understand that statement_timeout and
idle_in_transaction_session_timeout are probably never armed by this
worker, so functionally they may not need special handling.Hmm, but REPACK is not autovacuum; it's quite different in fact, in that
REPACK is intended to always be invoked manually, while autovacuum runs
on its own. On the other hand, because REPACK refuses to run in a
transaction block, transaction_timeout and
idle_in_transaction_session_timeout don't really apply, so I'm not
seeing the potential for problems.Yeah, I fully understood the difference. My concern was only about the inconsistency.
My concern is that the inconsistency might lead to confusion to future
readers. Does it make sense to either remove those two from
auto-vacuum worker or set them to repack worker as well?I decidedly don't want to touch autovacuum. Although I'm not sure I see
the reason why the transaction-based timeouts are relevant for
autovacuum.That was actually my concern. The fact that this raised the question of why autovacuum resets those timeouts suggests that the inconsistency can be confusing to readers.
I agree we don't need to touch autovacuum in this patch. Does it make sense to remove those unnecessary timeout resets from autovacuum by a separate patch?
Say, if another worker is added in the future, the author may look at both the autovacuum and repack workers as references, notice that they reset different sets of timeouts, and then have to spend time figuring out which behavior to follow and why.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Hi Alvaro, Chao,
Attached is v3. The code is the same as v2, with two additions.
1. The comment in RepackWorkerMain() now says why only lock_timeout and
transaction_timeout are passed. lock_timeout is armed by any lock wait,
and transaction_timeout by StartTransaction(). Both can fire in the
worker: the wait for older transactions in the snapshot builder is a
lock wait, and the worker's transaction spans the whole command.
statement_timeout and idle_in_transaction_session_timeout are only
armed by the command loop in PostgresMain(), which a background worker
never runs. I hope that answers Chao's question without touching
autovacuum.
2. A lock_timeout on the table owner role
does not reach the worker, and one set in the REPACK session does.
It fails on unpatched master and passes with 0001.
I expect this to be common in production, since a database- or
role-level lock_timeout is a normal setting. As REPACK (CONCURRENTLY)
is new in PG19, I suggest adding this to the open items and fixing it
before release.
Thanks,
Shihao
Attachments:
t253770_3v3-0001-Pass-the-backend-s-timeout-settings-to-the-REPACK.patchapplication/octet-stream; name=v3-0001-Pass-the-backend-s-timeout-settings-to-the-REPACK.patchDownload+27-1
v3-0002-Add-isolation-test-for-timeouts-in-the-REPACK-dec.patchapplication/octet-stream; name=v3-0002-Add-isolation-test-for-timeouts-in-the-REPACK-dec.patchDownload+62-1
On 2026-Sep-12, shihao zhong wrote:
1. The comment in RepackWorkerMain() now says why only lock_timeout and
transaction_timeout are passed. lock_timeout is armed by any lock wait,
and transaction_timeout by StartTransaction(). Both can fire in the
worker: the wait for older transactions in the snapshot builder is a
lock wait, and the worker's transaction spans the whole command.
statement_timeout and idle_in_transaction_session_timeout are only
armed by the command loop in PostgresMain(), which a background worker
never runs. I hope that answers Chao's question without touching
autovacuum.
Thanks. I pushed the patch, but didn't keep the detailed comment. I
don't think we need to explain the details of how these timeouts work in
this comment, and it may be easy for the comment to become outdated.
I added the word "relevant" to the comment, to try and satisfy Chao's
concern: a reader of this code trying to add a new worker will have to
figure out what does "relevant" mean in whatever case they're trying to
implement :-)
2. A lock_timeout on the table owner role does not reach the worker,
and one set in the REPACK session does. It fails on unpatched master
and passes with 0001.
Did you mean to reference the proposed new test in this paragraph? I
think so, but the text says something else. (I don't really know what
the text tries to convey). Anyway, I didn't push this test either,
because I think it's likely to fail randomly on slow buildfarm machines,
so we would have to adjust the timeouts, and that becomes a whack-a-mole
game that I'm not very interested in playing at this point.
Thanks for the report and fix,
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Before you were born your parents weren't as boring as they are now. They
got that way paying your bills, cleaning up your room and listening to you
tell them how idealistic you are." -- Charles J. Sykes' advice to teenagers