REPACK (CONCURRENTLY) can crash a logical decoding session
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.
This thread has been committed, so CI has stopped here. Anything below is the last result it produced.
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:t253645psql -h localhost -U postgresBuilt from patchset v6 (message #6), September 10, 2026 at 09:24 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 t253645_6 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 t253645_6 && git checkout t253645_6Patchset v6 (message #6) is on t253645_6
Hi,
I have been test-driving repack in an attempt to break it. I had no
luck, but I set Claude on a mission, and it reported the following.
<claude>
While stress-testing REPACK (CONCURRENTLY) on master (7d3247ccd15) I ran
into a server crash: a backend doing logical decoding segfaults while
decoding the transaction that a concurrent repack produced. It
reproduces on a non-assert build, at wal_level = replica and = logical.
Reproducer
----------
Session A:
CREATE TABLE t (id int PRIMARY KEY, big text);
INSERT INTO t SELECT g, 'small' FROM generate_series(1, 2000000) g;
SELECT pg_create_logical_replication_slot('s', 'test_decoding');
Session B, looping while the REPACK below runs. The value has to be
large and incompressible, so that the UPDATE stores a new out-of-line
TOAST value:
UPDATE t SET big = (SELECT string_agg(md5(id::text||i::text),'')
FROM generate_series(1,400) i)
WHERE id BETWEEN 10 AND 400;
Session A:
REPACK (CONCURRENTLY) t;
and then, once it has finished:
SELECT count(*) FROM pg_logical_slot_get_changes(
's', NULL, NULL, 'include-rewrites', '1');
server closed the connection unexpectedly
LOG: client backend (PID 2933076) was terminated by signal 11:
Segmentation fault
LOG: terminating any other active server processes
LOG: all server processes terminated; reinitializing
On an assert build it stops one frame earlier:
TRAP: failed Assert("change->data.tp.newtuple"),
File: "reorderbuffer.c", Line: 5144
ReorderBufferToastReplace
<- ReorderBufferProcessTXN <- ReorderBufferCommit
<- xact_decode <- LogicalDecodingProcessRecord
<- pg_logical_slot_get_changes
Analysis
--------
There seem to be two separate gaps in the TABLE_*_NO_LOGICAL plumbing
that 28d534e2ae0 added so that the transient heap's changes stay out of
the logical stream. INSERT and DELETE are covered; UPDATE is covered
only halfway. Individually neither gap is visible, but together they
produce the crash above.
1) The catch-up phase's TOAST rows are still logically logged.
heap_update() derives walLogical from TABLE_UPDATE_NO_LOGICAL and honours
it for the main tuple, but the TOAST call underneath passes a hardcoded
0 rather than the caller's options (heapam.c:3965):
if (need_toast)
{
/* Note we always use WAL and FSM during updates */
heaptup = heap_toast_insert_or_update(relation, newtup, &oldtup, 0);
The equivalent call on the insert path does pass options through
(heap_prepare_insert(), heapam.c:2265), so apply_concurrent_insert()
behaves as intended and apply_concurrent_update() does not. The
consequence is that the TOAST rows written into the transient heap's
TOAST relation during process_concurrent_changes() are decodable, and
every concurrent decoding session collects them into txn->toast_hash.
That happens regardless of include-rewrites, since the transient heap's
TOAST relation does not have relrewrite set.
2) A NO_LOGICAL update still queues a tuple-less change.
Not setting XLH_UPDATE_CONTAINS_NEW_TUPLE is not the same as suppressing
the record. DecodeDelete() got an explicit early return for the new
flag (decode.c:1056):
if (xlrec->flags & XLH_DELETE_NO_LOGICAL)
return;
DecodeUpdate() has no counterpart, so it still allocates a
REORDER_BUFFER_CHANGE_UPDATE with newtuple == NULL and oldtuple == NULL.
For an output plugin that does not ask for rewrites this is invisible,
because ReorderBufferProcessTXN() drops the change on the
relation->rd_rel->relrewrite test. With include-rewrites you can see
them directly:
table public.t: UPDATE: (no-tuple-data)
Put together, (1) leaves txn->toast_hash non-empty so
ReorderBufferToastReplace() no longer returns early on its
/* no toast tuples changed */
if (txn->toast_hash == NULL)
return;
and (2) hands it a change with no new tuple. The only thing between
that and heap_deform_tuple(NULL, ...) at reorderbuffer.c:5162 is the
assertion on line 5144.
Incidentally, reaching that assertion is what convinced me (1) is real:
the function cannot get there with an empty toast_hash. A repack whose
catch-up phase writes no out-of-line values does not crash; it just
emits the stray "(no-tuple-data)" records.
Scope
-----
The crash needs an output plugin that sets
OutputPluginOptions.receive_rewrites. In core that is only
test_decoding with include-rewrites, so built-in logical replication via
pgoutput is not affected; third-party plugins that ask for rewrites
would be. It is reachable by any user who can create a replication slot
and run REPACK (CONCURRENTLY), and it takes the whole cluster down with
it.
Fixes
-----
Either change alone stops the crash, but both look worth making.
For (1), just propagate the caller's options as the insert path does:
- heaptup = heap_toast_insert_or_update(relation, newtup, &oldtup, 0);
+ heaptup = heap_toast_insert_or_update(relation, newtup, &oldtup,
+ options);The comment above it ("Note we always use WAL and FSM during updates")
predates the feature and is no longer accurate, so it wants adjusting
too. This also stops other decoding sessions from reassembling TOAST
data that will only be thrown away.
For (2), mirror what was done for DELETE:
+ #define XLH_UPDATE_NO_LOGICAL (1<<7)
+ if (!walLogical)
+ xlrec.flags |= XLH_UPDATE_NO_LOGICAL; + if (xlrec->flags & XLH_UPDATE_NO_LOGICAL)
+ return; /* in DecodeUpdate() */Worth noting that xl_heap_update.flags is a uint8 and bits 0-6 are
already spoken for, so 1<<7 is the last one available. If that bit is
wanted for something else, the alternative is to make DecodeUpdate()
tolerate a missing new tuple the way DecodeInsert() already tolerates a
missing XLH_INSERT_CONTAINS_NEW_TUPLE, i.e. return early rather than
queue an empty change. That would arguably be worth doing anyway as
defence in depth, since ReorderBufferToastReplace()'s
Assert(change->data.tp.newtuple) is currently the only guard on a code
path a plugin can reach.
I have not looked at whether the same asymmetry can be reached without
REPACK; TABLE_UPDATE_NO_LOGICAL has no other caller today.
</claude>
Thom
Thom Brown <thom@linux.com> wrote:
I have been test-driving repack in an attempt to break it. I had no
luck, but I set Claude on a mission, and it reported the following.
TBH I usually fail to follow the "analysis" of LLMs (I found it rather
chaotic). Nevertheless, what you posted pointed my attention to an obvious
failure to pass the correct options to heap_toast_insert_or_update():
1) The catch-up phase's TOAST rows are still logically logged.
heap_update() derives walLogical from TABLE_UPDATE_NO_LOGICAL and honours
it for the main tuple, but the TOAST call underneath passes a hardcoded
0 rather than the caller's options (heapam.c:3965):if (need_toast)
{
/* Note we always use WAL and FSM during updates */
heaptup = heap_toast_insert_or_update(relation, newtup, &oldtup, 0);
Attached (0001) is a spec file for the isolation tester that reproduces the
crash reliably. It's a separate diff because I'm not sure it needs to be
merged.
This appears to be true - a special case that I have missed:
The crash needs an output plugin that sets
OutputPluginOptions.receive_rewrites.
Fixes
-----Either change alone stops the crash, but both look worth making.
For (1), just propagate the caller's options as the insert path does:
- heaptup = heap_toast_insert_or_update(relation, newtup, &oldtup, 0); + heaptup = heap_toast_insert_or_update(relation, newtup, &oldtup, + options);
This is not true. I didn't check (2), but (1) is wrong. The correct fix is
attached (0002).
Thanks a lot for your testing!
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
On Wed, 2 Sept 2026 at 19:19, Antonin Houska <ah@cybertec.at> wrote:
Thom Brown <thom@linux.com> wrote:
I have been test-driving repack in an attempt to break it. I had no
luck, but I set Claude on a mission, and it reported the following.TBH I usually fail to follow the "analysis" of LLMs (I found it rather
chaotic). Nevertheless, what you posted pointed my attention to an obvious
failure to pass the correct options to heap_toast_insert_or_update():1) The catch-up phase's TOAST rows are still logically logged.
heap_update() derives walLogical from TABLE_UPDATE_NO_LOGICAL and honours
it for the main tuple, but the TOAST call underneath passes a hardcoded
0 rather than the caller's options (heapam.c:3965):if (need_toast)
{
/* Note we always use WAL and FSM during updates */
heaptup = heap_toast_insert_or_update(relation, newtup, &oldtup, 0);Attached (0001) is a spec file for the isolation tester that reproduces the
crash reliably. It's a separate diff because I'm not sure it needs to be
merged.This appears to be true - a special case that I have missed:
The crash needs an output plugin that sets
OutputPluginOptions.receive_rewrites.Fixes
-----Either change alone stops the crash, but both look worth making.
For (1), just propagate the caller's options as the insert path does:
- heaptup = heap_toast_insert_or_update(relation, newtup, &oldtup, 0); + heaptup = heap_toast_insert_or_update(relation, newtup, &oldtup, + options);This is not true. I didn't check (2), but (1) is wrong. The correct fix is
attached (0002).Thanks a lot for your testing!
Thanks for taking a look. I have tested your fix and it no longer
crashes with the test case, so you appear to have resolved the
problem.
Regards
Thom
Hi,
On Wed, Sep 2, 2026 at 11:19 AM Antonin Houska <ah@cybertec.at> wrote:
Thom Brown <thom@linux.com> wrote:
I have been test-driving repack in an attempt to break it. I had no
luck, but I set Claude on a mission, and it reported the following.TBH I usually fail to follow the "analysis" of LLMs (I found it rather
chaotic). Nevertheless, what you posted pointed my attention to an obvious
failure to pass the correct options to heap_toast_insert_or_update():1) The catch-up phase's TOAST rows are still logically logged.
heap_update() derives walLogical from TABLE_UPDATE_NO_LOGICAL and honours
it for the main tuple, but the TOAST call underneath passes a hardcoded
0 rather than the caller's options (heapam.c:3965):if (need_toast)
{
/* Note we always use WAL and FSM during updates */
heaptup = heap_toast_insert_or_update(relation, newtup, &oldtup, 0);Attached (0001) is a spec file for the isolation tester that reproduces the
crash reliably. It's a separate diff because I'm not sure it needs to be
merged.This appears to be true - a special case that I have missed:
The crash needs an output plugin that sets
OutputPluginOptions.receive_rewrites.Fixes
-----Either change alone stops the crash, but both look worth making.
For (1), just propagate the caller's options as the insert path does:
- heaptup = heap_toast_insert_or_update(relation, newtup, &oldtup, 0); + heaptup = heap_toast_insert_or_update(relation, newtup, &oldtup, + options);This is not true. I didn't check (2), but (1) is wrong. The correct fix is
attached (0002).
Thank you for making the patches! I have one comment on 0001 patch:
+ if (!walLogical)
+ toast_options |= TABLE_INSERT_NO_LOGICAL;Given it's a heap operation, HEAP_INSERT_NO_LOGICAL would be appropriate.
The regression tests added by the 0001 patch looks good. I'd like to
merge them into one patch adding the test to Makefile and meson.build.
I'd suggest naming repack_decode.spec or something along those lines.
Regarding (2), I think it's worth fixing since it would lead to
passing an UPDATE change with neither old tuple nor new tuple to
output plugins. For instance, with test_decoding we would end up
showing:
table public.t: UPDATE: (no-tuple-data)
Which is undesirable for UPDATE changes. For fix, I don't think the
proposed approach is the right approach. It would be better to have
DecodeUpdate() ignore a change if it doesn't have the new tuple.
I've attached the updated patches. I merged Antonin's two patches into
one with some cosmetic changes and the 0002 patch fixes issue (2).
Please review them.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
Attachments:
t253645_4v2-0002-Fix-logical-decoding-to-ignore-updates-without-a-.patchtext/x-patch; charset=US-ASCII; name=v2-0002-Fix-logical-decoding-to-ignore-updates-without-a-.patchDownload+79-2
v2-0001-Fix-heap_update-ignoring-TABLE_UPDATE_NO_LOGICAL-.patchtext/x-patch; charset=US-ASCII; name=v2-0001-Fix-heap_update-ignoring-TABLE_UPDATE_NO_LOGICAL-.patchDownload+107-3
Hi
On Saturday, September 5, 2026 3:17 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
I've attached the updated patches. I merged Antonin's two patches into one
with some cosmetic changes and the 0002 patch fixes issue (2).
Please review them.
Both fixes look good to me. Just one question for the 0002.
+ /*
+ * Ignore update records without a new tuple. This happens when the
+ * caller of heap_update() asked for the change not to be decoded, as
+ * REPACK (CONCURRENTLY) does for the transient heap.
+ */
+ if (!(xlrec->flags & XLH_UPDATE_CONTAINS_NEW_TUPLE))
+ return;It seems to me that updates on catalog relations with no new tuple will also be
skipped after this patch. I think that's fine, but perhaps we could mention this
case in the comment to make the behavior clearer.
Best Regards,
Zhijie Hou
On Sun, Sep 6, 2026 at 9:18 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
Hi
On Saturday, September 5, 2026 3:17 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
I've attached the updated patches. I merged Antonin's two patches into one
with some cosmetic changes and the 0002 patch fixes issue (2).
Please review them.Both fixes look good to me. Just one question for the 0002.
+ /* + * Ignore update records without a new tuple. This happens when the + * caller of heap_update() asked for the change not to be decoded, as + * REPACK (CONCURRENTLY) does for the transient heap. + */ + if (!(xlrec->flags & XLH_UPDATE_CONTAINS_NEW_TUPLE)) + return;It seems to me that updates on catalog relations with no new tuple will also be
skipped after this patch. I think that's fine, but perhaps we could mention this
case in the comment to make the behavior clearer.
Good point. I've updated the comment accordingly and attached the
updated patches.
Also, I've fixed a whitespace issue in the 0001 patch.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
Attachments:
t253645_6v3-0001-Fix-heap_update-ignoring-TABLE_UPDATE_NO_LOGICAL-.patchtext/x-patch; charset=US-ASCII; name=v3-0001-Fix-heap_update-ignoring-TABLE_UPDATE_NO_LOGICAL-.patchDownload+106-3
v3-0002-Fix-logical-decoding-to-ignore-updates-without-a-.patchtext/x-patch; charset=US-ASCII; name=v3-0002-Fix-logical-decoding-to-ignore-updates-without-a-.patchDownload+81-2
On Tue, Sep 8, 2026 at 2:12 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Sun, Sep 6, 2026 at 9:18 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:Hi
On Saturday, September 5, 2026 3:17 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
I've attached the updated patches. I merged Antonin's two patches into one
with some cosmetic changes and the 0002 patch fixes issue (2).
Please review them.Both fixes look good to me. Just one question for the 0002.
+ /* + * Ignore update records without a new tuple. This happens when the + * caller of heap_update() asked for the change not to be decoded, as + * REPACK (CONCURRENTLY) does for the transient heap. + */ + if (!(xlrec->flags & XLH_UPDATE_CONTAINS_NEW_TUPLE)) + return;It seems to me that updates on catalog relations with no new tuple will also be
skipped after this patch. I think that's fine, but perhaps we could mention this
case in the comment to make the behavior clearer.Good point. I've updated the comment accordingly and attached the
updated patches.Also, I've fixed a whitespace issue in the 0001 patch.
The patches look good to me so I'm going to push them, barring any objections.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
On 2026-Sep-10, Masahiko Sawada wrote:
The patches look good to me so I'm going to push them, barring any
objections.
No objections here -- they look good to me too. The isolation spec
addition is great, thanks.
Regards
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Learn about compilers. Then everything looks like either a compiler or
a database, and now you have two problems but one of them is fun."
https://twitter.com/thingskatedid/status/1456027786158776329
On Fri, Sep 11, 2026 at 5:02 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:
On 2026-Sep-10, Masahiko Sawada wrote:
The patches look good to me so I'm going to push them, barring any
objections.No objections here -- they look good to me too. The isolation spec
addition is great, thanks.
Thank you for looking at the patches! Pushed.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
Import Notes
Reply to msg id not found: aqPtaPNZdskc83gS@alvherre.pgsql