REPACK (CONCURRENTLY) can crash a logical decoding session

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