Hi, On Wed, Sep 2, 2026 at 11:19 AM Antonin Houska <[email protected]> wrote: > > Thom Brown <[email protected]> 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
From 9d22c0faa034bcabdffb10607299bb429a60c4fb Mon Sep 17 00:00:00 2001 From: Masahiko Sawada <[email protected]> Date: Fri, 4 Sep 2026 22:27:51 -0700 Subject: [PATCH v2 2/2] Fix logical decoding to ignore updates without a new tuple. REPACK (CONCURRENTLY) suppresses logical decoding of the changes it applies to the transient heap. For an update, suppression keeps the tuple data out of the WAL record, but the record itself is still written, and decoding turned it into a change carrying neither a new nor an old tuple. An output plugin that asks for the changes made by heap rewrites therefore outputs an UPDATE with no data at all, reported under the name of the table being repacked. Ignore such records, as decoding already does for inserts; deletes have a WAL flag of their own for this. Backpatch to v19, where REPACK (CONCURRENTLY) was introduced. Reported-by: Thom Brown <[email protected]> Reviewed-by: Discussion: https://postgr.es/m/CAA-aLv7L_-dOuHXjLh0Di66dExdOb=uTOzR=jtrqcmv0wxy...@mail.gmail.com Backpatch-through: 19 --- src/backend/replication/logical/decode.c | 8 ++++ .../expected/repack_decode.out | 48 ++++++++++++++++++- .../injection_points/specs/repack_decode.spec | 24 ++++++++++ 3 files changed, 79 insertions(+), 1 deletion(-) diff --git a/src/backend/replication/logical/decode.c b/src/backend/replication/logical/decode.c index c944be4ac83..81bfe6b6b5b 100644 --- a/src/backend/replication/logical/decode.c +++ b/src/backend/replication/logical/decode.c @@ -981,6 +981,14 @@ DecodeUpdate(LogicalDecodingContext *ctx, XLogRecordBuffer *buf) xlrec = (xl_heap_update *) XLogRecGetData(r); + /* + * 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; + /* only interested in our database */ XLogRecGetBlockTag(r, 0, &target_locator, NULL, NULL); if (target_locator.dbOid != ctx->slot->data.database) diff --git a/src/test/modules/injection_points/expected/repack_decode.out b/src/test/modules/injection_points/expected/repack_decode.out index 0de24241bfb..b766ed544fe 100644 --- a/src/test/modules/injection_points/expected/repack_decode.out +++ b/src/test/modules/injection_points/expected/repack_decode.out @@ -26,7 +26,53 @@ step s1_decode: count ----- - 9 + 8 +(1 row) + +injection_points_detach +----------------------- + +(1 row) + +pg_drop_replication_slot +------------------------ + +(1 row) + + +starting permutation: s1_wait_before_lock s2_short_change s2_wakeup_before_lock s1_decode_updates +injection_points_attach +----------------------- + +(1 row) + +step s1_wait_before_lock: + REPACK (CONCURRENTLY) repack_toast; + <waiting ...> +step s2_short_change: + UPDATE repack_toast SET t = 'short' WHERE i=1; + +step s2_wakeup_before_lock: + SELECT injection_points_wakeup('repack-concurrently-before-lock'); + +injection_points_wakeup +----------------------- + +(1 row) + +step s1_wait_before_lock: <... completed> +step s1_decode_updates: + SELECT data FROM pg_logical_slot_peek_changes('s', NULL, NULL, 'include-rewrites', '1') + WHERE data LIKE '%UPDATE%'; + +data +--------------------------------------------------------------- +table public.repack_toast: UPDATE: i[integer]:1 t[text]:'short' +(1 row) + +injection_points_detach +----------------------- + (1 row) pg_drop_replication_slot diff --git a/src/test/modules/injection_points/specs/repack_decode.spec b/src/test/modules/injection_points/specs/repack_decode.spec index 31288cf75e3..da326fb6052 100644 --- a/src/test/modules/injection_points/specs/repack_decode.spec +++ b/src/test/modules/injection_points/specs/repack_decode.spec @@ -42,12 +42,28 @@ step s1_decode { SELECT count(*) FROM pg_logical_slot_peek_changes('s', NULL, NULL, 'include-rewrites', '1'); } +# Show the decoded updates. The row loaded by setup carries a random TOAST +# value, so only the updates are stable enough to display. +step s1_decode_updates +{ + SELECT data FROM pg_logical_slot_peek_changes('s', NULL, NULL, 'include-rewrites', '1') + WHERE data LIKE '%UPDATE%'; +} +teardown +{ + SELECT injection_points_detach('repack-concurrently-before-lock'); +} session s2 step s2_changes { UPDATE repack_toast SET t = gen_external() WHERE i=1; } +# Change the row using a value small enough to stay in-line. +step s2_short_change +{ + UPDATE repack_toast SET t = 'short' WHERE i=1; +} step s2_wakeup_before_lock { SELECT injection_points_wakeup('repack-concurrently-before-lock'); @@ -59,3 +75,11 @@ permutation s2_wakeup_before_lock s1_decode +# The changes REPACK applies to the transient heap must not be reported to an +# output plugin, not even as records that carry no tuple. Unlike the +# permutation above, no TOAST value is involved here. +permutation + s1_wait_before_lock + s2_short_change + s2_wakeup_before_lock + s1_decode_updates -- 2.55.0
From 319d73d301d6800c602617fe41661f48a4c257cf Mon Sep 17 00:00:00 2001 From: Antonin Houska <[email protected]> Date: Wed, 2 Sep 2026 19:17:53 +0200 Subject: [PATCH v2 1/2] Fix heap_update() ignoring TABLE_UPDATE_NO_LOGICAL for TOAST tuples. heap_update() honored TABLE_UPDATE_NO_LOGICAL when logging the main tuple, but not for the tuples it writes to the TOAST relation. Passing no options down to the tuple toaster was correct until updates gained the flag: inserts have propagated theirs ever since suppression was introduced for heap rewrites. REPACK (CONCURRENTLY) is the only user of the flag, and relies on it to keep the changes it applies to the transient heap out of the logical stream. Logical decoding therefore still reassembled the TOAST value of such an update, and then dereferenced the new tuple of a change that carries none, crashing the backend. This is reachable only if an output plugin asks for the changes made by heap rewrites. In core that is just test_decoding with include-rewrites. Backpatch to v19, where REPACK (CONCURRENTLY) was introduced. Reported-by: Thom Brown <[email protected]> Author: Antonin Houska <[email protected]> Reviewed-by: Masahiko Sawada <[email protected]> Discussion: https://postgr.es/m/CAA-aLv7L_-dOuHXjLh0Di66dExdOb=uTOzR=jtrqcmv0wxy...@mail.gmail.com Backpatch-through: 19 --- src/backend/access/heap/heapam.c | 8 ++- src/test/modules/injection_points/Makefile | 3 + .../expected/repack_decode.out | 36 +++++++++++ src/test/modules/injection_points/meson.build | 1 + .../injection_points/specs/repack_decode.spec | 61 +++++++++++++++++++ 5 files changed, 107 insertions(+), 2 deletions(-) create mode 100644 src/test/modules/injection_points/expected/repack_decode.out create mode 100644 src/test/modules/injection_points/specs/repack_decode.spec diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 72d6541734c..10766d330a9 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -3961,8 +3961,12 @@ l2: */ if (need_toast) { - /* Note we always use WAL and FSM during updates */ - heaptup = heap_toast_insert_or_update(relation, newtup, &oldtup, 0); + /* + * If logical decoding is not needed, suppress it for the TOAST + * tuples too. We never skip the FSM here. + */ + heaptup = heap_toast_insert_or_update(relation, newtup, &oldtup, + walLogical ? 0 : HEAP_INSERT_NO_LOGICAL); newtupsize = MAXALIGN(heaptup->t_len); } else diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile index 3b136adf126..c0d3956e147 100644 --- a/src/test/modules/injection_points/Makefile +++ b/src/test/modules/injection_points/Makefile @@ -9,6 +9,8 @@ EXTENSION = injection_points DATA = injection_points--1.0.sql PGFILEDESC = "injection_points - facility for injection points" +EXTRA_INSTALL = contrib/test_decoding + REGRESS = injection_points hashagg reindex_conc vacuum REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress @@ -16,6 +18,7 @@ ISOLATION = basic \ inplace \ reindex_concurrently_deferred \ repack \ + repack_decode \ repack_temporal \ repack_temporal_multirange \ repack_toast \ diff --git a/src/test/modules/injection_points/expected/repack_decode.out b/src/test/modules/injection_points/expected/repack_decode.out new file mode 100644 index 00000000000..0de24241bfb --- /dev/null +++ b/src/test/modules/injection_points/expected/repack_decode.out @@ -0,0 +1,36 @@ +Parsed test spec with 2 sessions + +starting permutation: s1_wait_before_lock s2_changes s2_wakeup_before_lock s1_decode +injection_points_attach +----------------------- + +(1 row) + +step s1_wait_before_lock: + REPACK (CONCURRENTLY) repack_toast; + <waiting ...> +step s2_changes: + UPDATE repack_toast SET t = gen_external() WHERE i=1; + +step s2_wakeup_before_lock: + SELECT injection_points_wakeup('repack-concurrently-before-lock'); + +injection_points_wakeup +----------------------- + +(1 row) + +step s1_wait_before_lock: <... completed> +step s1_decode: + SELECT count(*) FROM pg_logical_slot_peek_changes('s', NULL, NULL, 'include-rewrites', '1'); + +count +----- + 9 +(1 row) + +pg_drop_replication_slot +------------------------ + +(1 row) + diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build index aff516b901a..24372551796 100644 --- a/src/test/modules/injection_points/meson.build +++ b/src/test/modules/injection_points/meson.build @@ -47,6 +47,7 @@ tests += { 'inplace', 'reindex_concurrently_deferred', 'repack', + 'repack_decode', 'repack_temporal', 'repack_temporal_multirange', 'repack_toast', diff --git a/src/test/modules/injection_points/specs/repack_decode.spec b/src/test/modules/injection_points/specs/repack_decode.spec new file mode 100644 index 00000000000..31288cf75e3 --- /dev/null +++ b/src/test/modules/injection_points/specs/repack_decode.spec @@ -0,0 +1,61 @@ +setup +{ + CREATE EXTENSION injection_points; + + BEGIN; + -- Generate a string of random characters that is not likely to be + -- compressed, but is big enough to be stored externally. + CREATE FUNCTION gen_external() + RETURNS text + LANGUAGE sql as $$ + SELECT string_agg(chr(65 + trunc(25 * random())::int), '') + FROM generate_series(1, 2048) s(x); + $$; + COMMIT; + + SELECT pg_create_logical_replication_slot('s', 'test_decoding'); + + CREATE TABLE repack_toast(i int PRIMARY KEY, t text); + INSERT INTO repack_toast(i, t) VALUES (1, gen_external()); +} + +teardown +{ + DROP TABLE repack_toast; + DROP EXTENSION injection_points; + DROP FUNCTION gen_external(); + SELECT pg_drop_replication_slot('s'); +} + +session s1 +setup +{ + SELECT injection_points_set_local(); + SELECT injection_points_attach('repack-concurrently-before-lock', 'wait'); +} +# Perform the initial load and wait for s2 to do some data changes. +step s1_wait_before_lock +{ + REPACK (CONCURRENTLY) repack_toast; +} +step s1_decode +{ + SELECT count(*) FROM pg_logical_slot_peek_changes('s', NULL, NULL, 'include-rewrites', '1'); +} + +session s2 +step s2_changes +{ + UPDATE repack_toast SET t = gen_external() WHERE i=1; +} +step s2_wakeup_before_lock +{ + SELECT injection_points_wakeup('repack-concurrently-before-lock'); +} + +permutation + s1_wait_before_lock + s2_changes + s2_wakeup_before_lock + s1_decode + -- 2.55.0
