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

Hi,

On Wed, Sep 2, 2026 at 11:19 AM Antonin Houska <ah(at)cybertec(dot)at> wrote:
>
> 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).

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