From: Antonin Houska <ah@cybertec.at>
To: Thom Brown <thom@linux.com>
cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: REPACK (CONCURRENTLY) can crash a logical decoding session
In-reply-to: 
 <CAA-aLv7L_-dOuHXjLh0Di66dExdOb=uTOzR=jtrqCmV0Wxyd2Q@mail.gmail.com>
References: 
 <CAA-aLv7L_-dOuHXjLh0Di66dExdOb=uTOzR=jtrqCmV0Wxyd2Q@mail.gmail.com>
Comments: In-reply-to Thom Brown <thom@linux.com>
   message dated "Wed, 02 Sep 2026 12:48:15 +0100."
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="=-=-="
Date: Wed, 02 Sep 2026 20:19:21 +0200
Message-ID: <56617.1788373161@localhost>
Archived-At: 
 <https://www.postgresql.org/message-id/56617.1788373161%40localhost>
Precedence: bulk

--=-=-=
Content-Type: text/plain

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


--=-=-=
Content-Type: text/x-diff
Content-Disposition: attachment;
 filename=0001-Reproduce-failure-when-only-the-TOAST-tuple-is-logic.patch

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


--=-=-=
Content-Type: text/x-diff
Content-Disposition: attachment;
 filename=0002-Suppress-decoding-of-both-main-and-TOAST-tuple-in-RE.patch

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


--=-=-=--