Re: REPACK (CONCURRENTLY) can crash a logical decoding session

Thom Brown <thom(at)linux(dot)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