Horton

Re: refactoring relation extension and BufferAlloc(), faster COPY

Kyotaro Horiguchi <horikyota.ntt@gmail.com>

View thread View on postgr.es
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
To: andres@anarazel.de
Cc: hlinnaka@iki.fi, alvherre@alvh.no-ip.org, vignesh21@gmail.com, pgsql-hackers@postgresql.org, thomas.munro@gmail.com, melanieplageman@gmail.com, y.sokolov@postgrespro.ru, robertmhaas@gmail.com
Date: 2023-03-27T06:32:47Z
Lists: 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

Commits

  1. lwlock: Fix quadratic behavior with very long wait lists

  2. Support RBM_ZERO_AND_CLEANUP_LOCK in ExtendBufferedRelTo(), add tests

  3. Use ExtendBufferedRelTo() in XLogReadBufferExtended()

  4. hio: Use ExtendBufferedRelBy() to extend tables more efficiently

  5. heapam: Pass number of required pages to RelationGetBufferForTuple()

  6. Convert many uses of ReadBuffer[Extended](P_NEW) to ExtendBufferedRel()

  7. Use ExtendBufferedRelTo() in {vm,fsm}_extend()

  8. bufmgr: Introduce infrastructure for faster relation extension

  9. bufmgr: Support multiple in-progress IOs by using resowner

  10. bufmgr: Acquire and clean victim buffer separately

  11. bufmgr: Add Pin/UnpinLocalBuffer()

  12. bufmgr: Add some more error checking [infrastructure] around pinning

  13. Add smgrzeroextend(), FileZero(), FileFallocate()

  14. Don't initialize page in {vm,fsm}_extend(), not needed

  15. bufmgr: Remove buffer-write-dirty tracepoints

  16. hio: Release extension lock before initializing page / pinning VM

  17. bufmgr: Fix undefined behaviour with, unrealistically, large temp_buffers

  18. Fix memory leak and inefficiency in CREATE DATABASE ... STRATEGY WAL_LOG

  19. pgstat: Track more detailed relation IO statistics

  20. Use unnamed POSIX semaphores on Cygwin.

  21. Include RelFileLocator fields individually in BufferTag.