agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedREPACK (CONCURRENTLY) can crash a logical decoding session
9+ messages / 5 participants
[nested] [flat]
* REPACK (CONCURRENTLY) can crash a logical decoding session
@ 2026-09-02 11:48 Thom Brown <thom@linux.com>
0 siblings, 1 reply; 9+ messages in thread
From: Thom Brown @ 2026-09-02 11:48 UTC (permalink / raw)
To: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: REPACK (CONCURRENTLY) can crash a logical decoding session
@ 2026-09-02 18:19 Antonin Houska <ah@cybertec.at>
parent: Thom Brown <thom@linux.com>
0 siblings, 2 replies; 9+ messages in thread
From: Antonin Houska @ 2026-09-02 18:19 UTC (permalink / raw)
To: Thom Brown <thom@linux.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
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
Attachments:
[text/x-diff] 0001-Reproduce-failure-when-only-the-TOAST-tuple-is-logic.patch (3.5K, ../../56617.1788373161@localhost/2-0001-Reproduce-failure-when-only-the-TOAST-tuple-is-logic.patch)
download | inline diff:
From 96179fc6076c7ab0b8d3131e97f7c10f096aef82 Mon Sep 17 00:00:00 2001
From: Antonin Houska <ah@cybertec.at>
Date: Wed, 2 Sep 2026 19:17:53 +0200
Subject: [PATCH 1/2] Reproduce failure when only the TOAST tuple is logically
decoded.
The bug was introduced by commit 28d534e2ae, in which REPACK (CONCURRENTLY)
suppresses decoding of DML commands in the new heap during repacking. The
problem is that for UPDATE we only disabled decoding of the main tuple, but
not for its TOAST tuple(s).
---
.../expected/repack_toast_bug.out | 36 ++++++++++++
.../specs/repack_toast_bug.spec | 57 +++++++++++++++++++
2 files changed, 93 insertions(+)
create mode 100644 src/test/modules/injection_points/expected/repack_toast_bug.out
create mode 100644 src/test/modules/injection_points/specs/repack_toast_bug.spec
diff --git a/src/test/modules/injection_points/expected/repack_toast_bug.out b/src/test/modules/injection_points/expected/repack_toast_bug.out
new file mode 100644
index 00000000000..0de24241bfb
--- /dev/null
+++ b/src/test/modules/injection_points/expected/repack_toast_bug.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/specs/repack_toast_bug.spec b/src/test/modules/injection_points/specs/repack_toast_bug.spec
new file mode 100644
index 00000000000..a09fd8753e0
--- /dev/null
+++ b/src/test/modules/injection_points/specs/repack_toast_bug.spec
@@ -0,0 +1,57 @@
+setup
+{
+ SELECT pg_create_logical_replication_slot('s', 'test_decoding');
+
+ CREATE EXTENSION IF NOT EXISTS injection_points;
+
+ -- 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);
+ $$;
+
+ 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;
+ 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.52.0
[text/x-diff] 0002-Suppress-decoding-of-both-main-and-TOAST-tuple-in-RE.patch (1.5K, ../../56617.1788373161@localhost/3-0002-Suppress-decoding-of-both-main-and-TOAST-tuple-in-RE.patch)
download | inline diff:
From 2d936ccea561a9acb2e418f9b3b9c42e1372670f Mon Sep 17 00:00:00 2001
From: Antonin Houska <ah@cybertec.at>
Date: Wed, 2 Sep 2026 19:28:37 +0200
Subject: [PATCH 2/2] Suppress decoding of both main and TOAST tuple in REPACK
(CONCURRENTLY).
REPACK (CONCURRENTLY) suppresses logical decoding of data changes applied to
the new heap. Due to an oversight, the suppression was not propagated to the
TOAST relation in heap_update(). This can cause crash if another backend is
decoding the changes generated by REPACK. In particular,
ReorderBufferToastReplace() can end up with segfault when trying to add
TOASTed attributes to the new tuple which is actually NULL.
---
src/backend/access/heap/heapam.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 72d6541734c..1c4edc14395 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -3961,8 +3961,18 @@ l2:
*/
if (need_toast)
{
+ int toast_options = 0;
+
+ /*
+ * If logical decoding is not needed, make sure that neither TOAST
+ * changes are decoded.
+ */
+ if (!walLogical)
+ toast_options |= TABLE_INSERT_NO_LOGICAL;
+
/* Note we always use WAL and FSM during updates */
- heaptup = heap_toast_insert_or_update(relation, newtup, &oldtup, 0);
+ heaptup = heap_toast_insert_or_update(relation, newtup, &oldtup,
+ toast_options);
newtupsize = MAXALIGN(heaptup->t_len);
}
else
--
2.52.0
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: REPACK (CONCURRENTLY) can crash a logical decoding session
@ 2026-09-04 13:11 Thom Brown <thom@linux.com>
parent: Antonin Houska <ah@cybertec.at>
1 sibling, 0 replies; 9+ messages in thread
From: Thom Brown @ 2026-09-04 13:11 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: REPACK (CONCURRENTLY) can crash a logical decoding session
@ 2026-09-05 06:17 Masahiko Sawada <sawada.mshk@gmail.com>
parent: Antonin Houska <ah@cybertec.at>
1 sibling, 1 reply; 9+ messages in thread
From: Masahiko Sawada @ 2026-09-05 06:17 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: Thom Brown <thom@linux.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
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:
[text/x-patch] v2-0002-Fix-logical-decoding-to-ignore-updates-without-a-.patch (5.1K, ../../CAD21AoA6kx0D+2-qY+EiBv5DPwknPG1r5jEcGa3uGR-TQLZWCg@mail.gmail.com/2-v2-0002-Fix-logical-decoding-to-ignore-updates-without-a-.patch)
download | inline diff:
From 9d22c0faa034bcabdffb10607299bb429a60c4fb Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
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 <thom@linux.com>
Reviewed-by:
Discussion: https://postgr.es/m/CAA-aLv7L_-dOuHXjLh0Di66dExdOb=uTOzR=jtrqCmV0Wxyd2Q@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
[text/x-patch] v2-0001-Fix-heap_update-ignoring-TABLE_UPDATE_NO_LOGICAL-.patch (6.4K, ../../CAD21AoA6kx0D+2-qY+EiBv5DPwknPG1r5jEcGa3uGR-TQLZWCg@mail.gmail.com/3-v2-0001-Fix-heap_update-ignoring-TABLE_UPDATE_NO_LOGICAL-.patch)
download | inline diff:
From 319d73d301d6800c602617fe41661f48a4c257cf Mon Sep 17 00:00:00 2001
From: Antonin Houska <ah@cybertec.at>
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 <thom@linux.com>
Author: Antonin Houska <ah@cybertec.at>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Discussion: https://postgr.es/m/CAA-aLv7L_-dOuHXjLh0Di66dExdOb=uTOzR=jtrqCmV0Wxyd2Q@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
^ permalink raw reply [nested|flat] 9+ messages in thread
* RE: REPACK (CONCURRENTLY) can crash a logical decoding session
@ 2026-09-06 16:18 Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
parent: Masahiko Sawada <sawada.mshk@gmail.com>
0 siblings, 1 reply; 9+ messages in thread
From: Zhijie Hou (Fujitsu) @ 2026-09-06 16:18 UTC (permalink / raw)
To: Masahiko Sawada <sawada.mshk@gmail.com>; +Cc: Thom Brown <thom@linux.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Antonin Houska <ah@cybertec.at>
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: REPACK (CONCURRENTLY) can crash a logical decoding session
@ 2026-09-08 21:12 Masahiko Sawada <sawada.mshk@gmail.com>
parent: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
0 siblings, 1 reply; 9+ messages in thread
From: Masahiko Sawada @ 2026-09-08 21:12 UTC (permalink / raw)
To: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; +Cc: Thom Brown <thom@linux.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Antonin Houska <ah@cybertec.at>
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:
[text/x-patch] v3-0001-Fix-heap_update-ignoring-TABLE_UPDATE_NO_LOGICAL-.patch (6.5K, ../../CAD21AoCdrxX26j1cw1MDyVMy25F=hmnJTSrm0_S9cVnERwrH3w@mail.gmail.com/2-v3-0001-Fix-heap_update-ignoring-TABLE_UPDATE_NO_LOGICAL-.patch)
download | inline diff:
From 1f1bed80244b916a5ce5b8e9a2b1980b2e29ea9a Mon Sep 17 00:00:00 2001
From: Antonin Houska <ah@cybertec.at>
Date: Wed, 2 Sep 2026 19:17:53 +0200
Subject: [PATCH v3 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 <thom@linux.com>
Author: Antonin Houska <ah@cybertec.at>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Reviewed-by: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
Discussion: https://postgr.es/m/CAA-aLv7L_-dOuHXjLh0Di66dExdOb=uTOzR=jtrqCmV0Wxyd2Q@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 | 60 +++++++++++++++++++
5 files changed, 106 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..89b4cbcc77e
--- /dev/null
+++ b/src/test/modules/injection_points/specs/repack_decode.spec
@@ -0,0 +1,60 @@
+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
[text/x-patch] v3-0002-Fix-logical-decoding-to-ignore-updates-without-a-.patch (5.3K, ../../CAD21AoCdrxX26j1cw1MDyVMy25F=hmnJTSrm0_S9cVnERwrH3w@mail.gmail.com/3-v3-0002-Fix-logical-decoding-to-ignore-updates-without-a-.patch)
download | inline diff:
From cbe21698cce9c27d09e0105cf71a768cb0bb5241 Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Fri, 4 Sep 2026 22:27:51 -0700
Subject: [PATCH v3 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. Updates on
catalog relations don't carry the new tuple either, so we ignore them
too. For deletes, REPACK sets XLH_DELETE_NO_LOGICAL instead.
Backpatch to v19, where REPACK (CONCURRENTLY) was introduced.
Reported-by: Thom Brown <thom@linux.com>
Reviewed-by: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
Discussion: https://postgr.es/m/CAA-aLv7L_-dOuHXjLh0Di66dExdOb=uTOzR=jtrqCmV0Wxyd2Q@mail.gmail.com
Backpatch-through: 19
---
src/backend/replication/logical/decode.c | 9 ++++
.../expected/repack_decode.out | 48 ++++++++++++++++++-
.../injection_points/specs/repack_decode.spec | 25 ++++++++++
3 files changed, 81 insertions(+), 1 deletion(-)
diff --git a/src/backend/replication/logical/decode.c b/src/backend/replication/logical/decode.c
index c944be4ac83..4a739230264 100644
--- a/src/backend/replication/logical/decode.c
+++ b/src/backend/replication/logical/decode.c
@@ -981,6 +981,15 @@ DecodeUpdate(LogicalDecodingContext *ctx, XLogRecordBuffer *buf)
xlrec = (xl_heap_update *) XLogRecGetData(r);
+ /*
+ * Ignore update records without a new tuple. This happens when the update
+ * is done on a catalog relation, or 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 89b4cbcc77e..c3efd9987b3 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');
@@ -58,3 +74,12 @@ permutation
s2_changes
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: REPACK (CONCURRENTLY) can crash a logical decoding session
@ 2026-09-10 17:37 Masahiko Sawada <sawada.mshk@gmail.com>
parent: Masahiko Sawada <sawada.mshk@gmail.com>
0 siblings, 1 reply; 9+ messages in thread
From: Masahiko Sawada @ 2026-09-10 17:37 UTC (permalink / raw)
To: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; +Cc: Thom Brown <thom@linux.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Antonin Houska <ah@cybertec.at>
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: REPACK (CONCURRENTLY) can crash a logical decoding session
@ 2026-09-11 12:02 Álvaro Herrera <alvherre@kurilemu.de>
parent: Masahiko Sawada <sawada.mshk@gmail.com>
0 siblings, 1 reply; 9+ messages in thread
From: Álvaro Herrera @ 2026-09-11 12:02 UTC (permalink / raw)
To: Masahiko Sawada <sawada.mshk@gmail.com>; +Cc: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; Thom Brown <thom@linux.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Antonin Houska <ah@cybertec.at>
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: REPACK (CONCURRENTLY) can crash a logical decoding session
@ 2026-09-11 17:56 Masahiko Sawada <sawada.mshk@gmail.com>
parent: Álvaro Herrera <alvherre@kurilemu.de>
0 siblings, 0 replies; 9+ messages in thread
From: Masahiko Sawada @ 2026-09-11 17:56 UTC (permalink / raw)
To: Álvaro Herrera <alvherre@kurilemu.de>; +Cc: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; Thom Brown <thom@linux.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Antonin Houska <ah@cybertec.at>
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
^ permalink raw reply [nested|flat] 9+ messages in thread
end of thread, other threads:[~2026-09-11 17:56 UTC | newest]
Thread overview: 9+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-09-02 11:48 REPACK (CONCURRENTLY) can crash a logical decoding session Thom Brown <thom@linux.com>
2026-09-02 18:19 ` Antonin Houska <ah@cybertec.at>
2026-09-04 13:11 ` Thom Brown <thom@linux.com>
2026-09-05 06:17 ` Masahiko Sawada <sawada.mshk@gmail.com>
2026-09-06 16:18 ` Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
2026-09-08 21:12 ` Masahiko Sawada <sawada.mshk@gmail.com>
2026-09-10 17:37 ` Masahiko Sawada <sawada.mshk@gmail.com>
2026-09-11 12:02 ` Álvaro Herrera <alvherre@kurilemu.de>
2026-09-11 17:56 ` Masahiko Sawada <sawada.mshk@gmail.com>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox