Hi,
On 2026-02-09 12:42:25 +0100, Antonin Houska wrote:
> Andres Freund <[email protected]> wrote:
>
> > On 2026-01-12 12:45:03 -0500, Andres Freund wrote:
> > > I'm doing another pass through 0003 and will push that if I don't find
> > > anything significant.
> >
> > Done, after adjust two comments in minor ways.
>
> I suppose this is commit 0b96e734c590.
>
> While troubleshooting REPACK issue [1], I realized that
> HeapTupleSatisfiesMVCCBatch() can also be called during logical decoding - in
> that case we need to use a historic MVCC snapshot.
Huh. Indeed. That's unintentional - the path should never have been reached,
we are checking that an MVCC snapshot is used. Unfortunately, somebody
(i.e. probably me) at some point defined the relevant macro as
/* This macro encodes the knowledge of which snapshots are MVCC-safe */
#define IsMVCCSnapshot(snapshot) \
((snapshot)->snapshot_type == SNAPSHOT_MVCC || \
(snapshot)->snapshot_type == SNAPSHOT_HISTORIC_MVCC)
Which makes sense for some places, but not for plenty others.
The reason this didn't cause more widespread issues is that during logical
decoding we mostly don't use sequential scans etc that are affected by the
these paths.
> My proposal to fix the problem is attached.
That's imo not at all the right fix - it'd make visibility during seqscans
checking noticeably slower.
I think we ought to instead restrict the page-at-a-time scans to only happen
with "real" mvcc snapshots. I.e. this:
/*
* Disable page-at-a-time mode if it's not a MVCC-safe snapshot.
*/
if (!(snapshot && IsMVCCSnapshot(snapshot)))
scan->rs_base.rs_flags &= ~SO_ALLOW_PAGEMODE;
should trigger for historic snapshots as well.
Does that fix the issue for you?
What's your reproducer?
Greetings,
Andres Freund
