On Sat, Jul 20, 2024 at 03:01:31PM +0300, Nazir Bilal Yavuz wrote: > On Sat, 20 Jul 2024 at 14:27, Noah Misch <n...@leadboat.com> wrote: > > > > On Thu, Jul 18, 2024 at 02:11:13PM +0300, Nazir Bilal Yavuz wrote: > > > v4 is attached. > > > > Removal of the PinBufferForBlock() comment about the "persistence = > > RELPERSISTENCE_PERMANENT" fallback started to feel like a loss. I looked > > for > > a way to re-add a comment about the fallback. > > https://coverage.postgresql.org/src/backend/storage/buffer/bufmgr.c.gcov.html > > shows no test coverage of that fallback, and I think the fallback is > > unreachable. Hence, I've removed the fallback in a separate commit. I've > > pushed that and your three patches. Thanks. > > Thanks for the separate commit and push! > > With the separate commit (e00c45f685), does it make sense to rename > the smgr_persistence parameter of the ReadBuffer_common() to > persistence? Because, ExtendBufferedRelTo() calls ReadBuffer_common() > with rel's persistence now, not with smgr's persistence.
BMR_REL() doesn't set relpersistence, so bmr.relpersistence is associated with bmr.smgr and is unset if bmr.rel is set. That is to say, bmr.relpersistence is an smgr_persistence. It could make sense to change ReadBuffer_common() to take a BufferManagerRelation instead of the three distinct arguments. On a different naming topic, my review missed that field name copy_storage_using_buffer_read_stream_private.last_block doesn't fit how the field is used. Code uses it like an nblocks. So let's either rename the field or change the code to use it as a last_block (e.g. initialize it to nblocks-1, not nblocks).
