Date: Thu, 20 Mar 2025 11:19:04 -0700
From: Noah Misch <noah@leadboat.com>
To: Andres Freund <andres@anarazel.de>
Cc: Antonin Houska <ah@cybertec.at>, pgsql-hackers@postgresql.org,
	Thomas Munro <thomas.munro@gmail.com>,
	Heikki Linnakangas <hlinnaka@iki.fi>,
	Robert Haas <robertmhaas@gmail.com>,
	Jakub Wartak <jakub.wartak@enterprisedb.com>,
	Jelte Fennema-Nio <postgres@jeltef.nl>
Subject: Re: AIO v2.5
Message-ID: <20250320181904.8a.nmisch@google.com>
References: <20250312035743.f5.nmisch@google.com>
 <w6uiicyou7hzq47mbyejubtcyb2rngkkf45fk4q7inue5kfbeo@bbfad3qyubvs>
 <4b3f32ug3cayekysqlgspz2qjmeb7lca3gvazayglxr2m3d4dv@il33accgsji7>
 <17906.1741863183@localhost>
 <3yxd5r23zly5bytvgyktbxtxq2r3gbpi7xd4dugevh3h4w4q6c@lu6oatjjpltz>
 <oav52ytakcvfnk52w5o5wvxdjavxth2tgptxt25gnfmh2pdcpk@omp3xukdu5if>
 <zp7obmaqdd6b5u2vhpw27hovpjirtqvjakkbkaz4vju7tohyul@4kfobe7ipgxm>
 <20250319212530.80.nmisch@google.com>
 <cz6eu7prqdibz35nljpqwyzyp37afncsp4r3fk4lkssiecdjck@vwew2c2vs2ar>
 <xfz5odj2mad6hioqzmblb6dswnq7izw2bsy4n5ka6w7yxkigq6@z7kujxypo2xu>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <xfz5odj2mad6hioqzmblb6dswnq7izw2bsy4n5ka6w7yxkigq6@z7kujxypo2xu>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20250320181904.8a.nmisch%40google.com>
Precedence: bulk

On Thu, Mar 20, 2025 at 01:05:05PM -0400, Andres Freund wrote:
> On 2025-03-19 18:17:37 -0400, Andres Freund wrote:
> > On 2025-03-19 14:25:30 -0700, Noah Misch wrote:
> > > > +		 * marked as failed. In case of a partial read, some buffers may be
> > > > +		 * ok.
> > > > +		 */
> > > > +		failed =
> > > > +			prior_result.status == ARS_ERROR
> > > > +			|| prior_result.result <= buf_off;
> > >
> > > I didn't run an experiment to check the following, but I think this should be
> > > s/<=/</.  Suppose we requested two blocks and read some amount of bytes
> > > [1*BLCKSZ, 2*BLSCKSZ - 1].  md_readv_complete will store result=1.  buf_off==0
> > > should compute failed=false here, but buf_off==1 should compute failed=true.
> > 
> > Huh, you might be right. I thought I wrote a test for this, I wonder why it
> > didn't catch the problem...
> 
> It was correct as-is. With result=1 you get precisely the result you describe
> as the desired outcome, no?
>    prior_result.result <= buf_off
>    ->
>    1 <= 0 -> failed = 0
>    1 <= 1 -> failed = 1
> 
> but if it were < as you suggest:
> 
>    prior_result.result < buf_off
>    ->
>    1 < 0 -> failed = 0
>    1 < 1 -> failed = 0
> 
> I.e. we would assume that the second buffer also completed.

That's right.  I see it now.  My mistake.

> What does concern me is that the existing tests do *not* catch the problem if
> I turn "<=" into "<".  The second buffer in this case wrongly gets marked as
> valid. We do retry the read (because bufmgr.c thinks only one block was read),
> but find the buffer to already be valid.
> 
> The reason the test doesn't fail, is that the way I set up the "short read"
> tests. The injection point runs after the IO completed and just modifies the
> result. However, the actual buffer contents still got modified.
> 
> 
> The easiest way around that seems to be to have the injection point actually
> zero out the remaining memory.

Sounds reasonable and sufficient.

FYI, I've resumed the comprehensive review.  That's still ongoing.