Re: BUG #19556: Segmentation fault in test_decoding

On Wed, Jul 22, 2026 at 2:13 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Wed, Jul 22, 2026 at 11:37 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Wed, Jul 22, 2026 at 8:22 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > >
> > > While researching this bug, I found another one that is related and
> > > probably should be fixed first: even in PG17 and earlier, logical
> > > decoding calls the commit_prepared callback without first calling the
> > > prepare callback:
> > >
> > > BEGIN;
> > > SELECT * FROM test WHERE id = 1 FOR SHARE;
> > > PREPARE TRANSACTION 'p1';
> > > COMMIT PREPARED 'p1';
> > >
> > > In logical replication, the subscriber ends up with an error because
> > > the prepared transaction doesn't exist on it.
> > >
> > > In summary, with the above scenario, logical decoding calls:
> > >
> > > - the prepare and commit_prepared callbacks (PG18+)
> > > - the commit_prepared callback (PG17-)
> > >
> > > Neither is correct.
> > >
> > > I think we shouldn't call the commit_prepared callback for an empty
> > > transaction (one that has no base snapshot), so in this case we
> > > shouldn't call any of the begin_prepare, prepare, or commit_prepared
> > > callbacks. Thoughts?
> > >
> >
> > I agree that when base_snapshot is not set, we shouldn't send these
> > transaction commands. I checked HEAD and it seems below change in
> > commit 072ee847ad lead to sending prepare:
> > - if (txn->concurrent_abort && !rbtxn_is_streamed(txn))
> > + if (!rbtxn_sent_prepare(txn))
> > + {
> > rb->prepare(rb, txn, txn->final_lsn);
> > + txn->txn_flags |= RBTXN_SENT_PREPARE;
> > + }
> >
> > Why did we remove the check of the aborted xact?

Commit 072ee847 replaced txn->concurrent_abort flag with
RBTXN_IS_ABORTED and this flag was used to check if we have sent a
prepare message for the transaction. We thought it can be achieved by
directly checking the RBTXN_SENT_PREPARE instead.

> >
> > I'll check PG17 and share my findings with you.
> >
>
> For PG17 and before, I think we can skip replaying commit if
> base_snapshot is not set similar to ReorderBufferReplay(). See
> attached. The other possibility is to update ReorderBufferReplay() to
> retrun a special value so that callers can skip sending commit or
> prepare.

Thank you for the patch. I like the approach the proposed patch does.
I've made a patch for PG18+ that fixes both issues with regression
tests. For PG17 or earlier, the patch doesn't need the changes in
ReorderBufferPrepare() but has the same regression tests.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com