Re: refactoring relation extension and BufferAlloc(), faster COPY - Mailing list pgsql-hackers

At Sun, 26 Mar 2023 12:26:59 -0700, Andres Freund <andres@anarazel.de> wrote in 
> Hi,
> 
> Attached is v5. Lots of comment polishing, a bit of renaming. I extracted the
> relation extension related code in hio.c back into its own function.
> 
> While reviewing the hio.c code, I did realize that too much stuff is done
> while holding the buffer lock. See also the pre-existing issue
> https://postgr.es/m/20230325025740.wzvchp2kromw4zqz%40awork3.anarazel.de

0001, 0002 looks fine to me.

0003 adds the new function FileFallocte, but we already have
AllocateFile. Although fd.c contains functions with varying word
orders, it could be confusing that closely named functions have
different naming conventions.

+    /*
+     * Return in cases of a "real" failure, if fallocate is not supported,
+     * fall through to the FileZero() backed implementation.
+     */
+    if (returnCode != EINVAL && returnCode != EOPNOTSUPP)
+        return returnCode;

I'm not entirely sure, but man 2 fallocate tells that ENOSYS also can
be returned. Some googling indicate that ENOSYS might need the same
amendment to EOPNOTSUPP. However, I'm not clear on why man
posix_fallocate donsn't mention the former.


+        (returnCode != EINVAL && returnCode != EINVAL))
:)


FileGetRawDesc(File file)
 {
     Assert(FileIsValid(file));
+
+    if (FileAccess(file) < 0)
+        return -1;
+

The function's comment is provided below.

> * The returned file descriptor will be valid until the file is closed, but
> * there are a lot of things that can make that happen.  So the caller should
> * be careful not to do much of anything else before it finishes using the
> * returned file descriptor.

So, the responsibility to make sure the file is valid seems to lie
with the callers, although I'm not sure since there aren't any
function users in the tree. I'm unclear as to why FileSize omits the
case lruLessRecently != file. When examining similar functions, such
as FileGetRawFlags and FileGetRawMode, I'm puzzled to find that
FileAccess() nor BasicOpenFilePermthe don't set the struct members
referred to by the functions.  This makes my question the usefulness
of these functions including FileGetRawDesc().  Regardless, since the
patchset doesn't use FileGetRawDesc(), I don't believe the fix is
necessary in this patch set.

+    if ((uint64) blocknum + nblocks >= (uint64) InvalidBlockNumber)

I'm not sure it is appropriate to assume InvalidBlockNumber equals
MaxBlockNumber + 1 in this context.


+        int            segstartblock = curblocknum % ((BlockNumber) RELSEG_SIZE);
+        int            segendblock = (curblocknum % ((BlockNumber) RELSEG_SIZE)) + remblocks;
+        off_t        seekpos = (off_t) BLCKSZ * segstartblock;

segendblock can be defined as "segstartblock + remblocks", which would
be clearer.

+         * If available and useful, use posix_fallocate() (via FileAllocate())

FileFallocate()?


+         * However, we don't use FileAllocate() for small extensions, as it
+         * defeats delayed allocation on some filesystems. Not clear where
+         * that decision should be made though? For now just use a cutoff of
+         * 8, anything between 4 and 8 worked OK in some local testing.

The chose is quite similar to what FileFallocate() makes. However, I'm
not sure FileFallocate() itself should be doing this.



regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center