MIME-Version: 1.0
References: 
 <c4ef1737-8cba-458e-b6fd-4e2d6011e985.duankunren.dkr@alibaba-inc.com>
 <18a40051-abc2-4fc2-8dcc-4dc39aa3e79e@iki.fi>
 <F513DFBB-DCFE-4B99-B01E-DDB04414857C@yandex-team.ru>
 <452355ef-7c20-4d96-88a2-8fbb49737dc9@iki.fi>
 <BD37E553-A017-4D2C-8203-702378BF5882@yandex-team.ru>
 <5b7f0a04-4a60-44bb-9d2c-8917af0b10fa@iki.fi>
 <C44AF20B-7AA3-42A6-B30D-08DEFD2E3F8C@yandex-team.ru>
 <33319276-e4d0-4773-89e4-09084905fdb0@iki.fi>
 <CALdSSPghO2YGyy4BN_rRxdG7dBSf1-pmi2D4NARk14+HyZoHhg@mail.gmail.com>
In-Reply-To: 
 <CALdSSPghO2YGyy4BN_rRxdG7dBSf1-pmi2D4NARk14+HyZoHhg@mail.gmail.com>
From: Kirill Reshke <reshkekirill@gmail.com>
Date: Sun, 22 Mar 2026 19:22:30 +0500
Message-ID: 
 <CALdSSPh2tfxcZOgdeGdjBwdnUBEY+8iG7unuJMEoruWToUMYwA@mail.gmail.com>
Subject: Re: Bug in MultiXact replay compat logic for older minor version
 after crash-recovery
To: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Andrey Borodin <x4mmm@yandex-team.ru>,
	=?UTF-8?B?5q615Z2k5LuBKOWIu+mfpyk=?= <duankunren.dkr@alibaba-inc.com>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CALdSSPh2tfxcZOgdeGdjBwdnUBEY%2B8iG7unuJMEoruWToUMYwA%40mail.gmail.com>
Precedence: bulk

On Sun, 22 Mar 2026 at 19:15, Kirill Reshke <reshkekirill@gmail.com> wrote:
>
> Hi!
> I can see that the back-branches commit was included into master [0].
> I think this is good.
>
> On Sun, 22 Mar 2026 at 16:10, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> >
> > On 20/03/2026 19:05, Andrey Borodin wrote:
> > >> On 20 Mar 2026, at 18:14, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> > >>
> > >> Zeroing the page again is dangerous because the CREATE_ID records can be out of order. The page might already contain some later multixids, and zeroing will overwrite them.
> > >
> > > I see only cases when it's not a problem: we zeroed page, did not flush it, thus did not extend the file, crashed, tested FS, zeroed page once more, overwrote again by replaying WAL, no big deal.
> > > We should never zero a page with offsets, that will not be replayed by WAL.
> >
> > I think we're in agreement, but I want to verify because this is
> > important to get right. I was replying to this:
> >
> > > If we are sure buffers have no this page we can detect it via FS.
> > > Otherwise... nothing bad can happen, actually. We might get false positive and zero the page once more.
> >
> > My point is that if we rely on SimpleLruDoesPhysicalPageExist(), and it
> > ever returns false even though we had already initialized the page, you
> > can lose data. It's *not* ok to zero a page again that was zeroed
> > earlier already, because we might have already written some real data on it.
>
> +1. Even if we manage to compose a "fix" that zeroes a page more than
> once, this "fix" will be non-future-profing and we will corrupt the
> database if anything goes even slightly wrong.
>
> > Let's consider this wal stream, generated with old minor version:
> >
> > ZERO_PAGE:2048 -> CREATE_ID:2048 -> CREATE_ID:2049 -> CREATE_ID:2047
> >
> > 2048 is the first multixid on the page. When WAL replay gets to the
> > CREATE_ID:2047 record, it will enter the backwards-compatibility
> > codepath and needs to determine if the page containing the next mxid
> > (2048) already exists.
> >
> > In this WAL sequence, the page already exist because the ZERO_PAGE
> > record was replayed earlier. But if we just call
> > SimpleLruDoesPhysicalPageExist(), it will return 'false' because the
> > page was not flushed to disk yet. If we believe that and zero the page
> > again, we will lose data (the offset for mxid 2049).
> >
> > The opposite cannot happen: if SimpleLruDoesPhysicalPageExist() returns
> > true, then it does really exist.
> >
> > So indeed we can only trust SimpleLruDoesPhysicalPageExist() if we are
> > sure that the page is not sitting in the buffers.
>
> +1
>
> > Attached is a new version. I updated the comment to explain that.
> >
> > I also added another safety measure: before calling
> > SimpleLruDoesPhysicalPageExist(), flush all the SLRU buffers. That way,
> > SimpleLruDoesPhysicalPageExist() should definitely return the correct
> > answer. That shouldn't be necessary because the check with
> > last_initialized_offsets_page should cover all the cases where a page
> > that extended the file is sitting in the buffers, but better safe than
> > sorry.
> >
> > - Heikki
>
> I played with v2 and was unable to fool it into corrupting db. So v2
> looks good to me.
>
>
> [0] https://git.postgresql.org/cgit/postgresql.git/commit/?id=516310ed4dba89bd300242df0d56b4782f33ed4d
>
> --
> Best regards,
> Kirill Reshke

Also, in commit message:

> the backwards compatibility logic to tolerate WAL generated by older minor versions

Let's define older as pre-789d65364c to be exact?

-- 
Best regards,
Kirill Reshke