Hi, On 2023-01-20 13:40:55 +1300, David Rowley wrote: > On Tue, 10 Jan 2023 at 15:08, Andres Freund <and...@anarazel.de> wrote: > > Thanks for letting me now. Updated version attached. > > I'm not too sure I've qualified for giving a meaningful design review > here, but I have started looking at the patches and so far only made > it as far as 0006.
Thanks! > I noted down the following while reading: > > v2-0001: > > 1. BufferCheckOneLocalPin needs a header comment > > v2-0002: > > 2. The following comment and corresponding code to release the > extension lock has been moved now. > > /* > * Release the file-extension lock; it's now OK for someone else to extend > * the relation some more. > */ > > I think it's worth detailing out why it's fine to release the > extension lock in the new location. You've added detail to the commit > message but I think you need to do the same in the comments too. Will do. > v2-0003 > > 3. FileFallocate() and FileZero() should likely document what they > return, i.e zero on success and non-zero on failure. I guess I just tried to fit in with the rest of the file :) > 4. I'm not quite clear on why you've modified FileGetRawDesc() to call > FileAccess() twice. I do not have the faintest idea what happened there... Will fix. > v2-0004: > > 5. Is it worth having two versions of PinLocalBuffer() one to adjust > the usage count and one that does not? Couldn't the version that does > not adjust the count skip doing pg_atomic_read_u32()? I think it'd be nicer to just move the read inside the if (adjust_usagecount). That way the rest of the function doesn't have to be duplicated. Thanks, Andres Freund
