On Thu, Mar 20, 2025 at 02:54:14PM -0400, Andres Freund wrote: > On 2025-03-19 18:11:18 -0700, Noah Misch wrote: > > On Wed, Mar 19, 2025 at 06:17:37PM -0400, Andres Freund wrote: > > > On 2025-03-19 14:25:30 -0700, Noah Misch wrote: > > > > I see this relies on md_readv_complete having converted "result" to > > > > blocks. > > > > Was there some win from doing that as opposed to doing the division > > > > here? > > > > Division here ("blocks_read = prior_result.result / BLCKSZ") would feel > > > > easier > > > > to follow, to me. > > > > > > It seemed like that would be wrong layering - what if we had an smgr that > > > could store data in a compressed format? The raw read would be of a > > > smaller > > > size. The smgr API deals in BlockNumbers, only the md.c layer should know > > > about bytes. > > > > I hadn't thought of that. That's a good reason. > > I thought that was better documented, but alas, it wasn't. How about updating > the documentation of smgrstartreadv to the following: > > /* > * smgrstartreadv() -- asynchronous version of smgrreadv() > * > * This starts an asynchronous readv IO using the IO handle `ioh`. Other than > * `ioh` all parameters are the same as smgrreadv(). > * > * Completion callbacks above smgr will be passed the result as the number of > * successfully read blocks if the read [partially] succeeds. This maintains > * the abstraction that smgr operates on the level of blocks, rather than > * bytes. > */
That's good. Possibly add "(Buffers for blocks not successfully read might bear unspecified modifications, up to the full nblocks.)" In a bit of over-thinking this, I wondered if shared_buffer_readv_complete would be better named shared_buffer_smgrreadv_complete, to emphasize the smgrreadv semantics. PGAIO_HCB_SHARED_BUFFER_READV likewise. But I tend to think not. smgrreadv() has no "result" concept, so the symmetry is limited. > I briefly had a bug in test_aio's injection point that lead to *increasing* > the number of bytes successfully read. That triggered an assertion failure in > bufmgr.c, but not closer to the problem. Is it worth adding an assert against > that to md_readv_complete? Can't quite decide. I'd lean yes, if in doubt.