Horton

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

Heikki Linnakangas <hlinnaka@iki.fi>

View thread View on postgr.es
From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Andres Freund <andres@anarazel.de>
Cc: vignesh C <vignesh21@gmail.com>, pgsql-hackers@postgresql.org, Thomas Munro <thomas.munro@gmail.com>, Melanie Plageman <melanieplageman@gmail.com>, Yura Sokolov <y.sokolov@postgrespro.ru>, Robert Haas <robertmhaas@gmail.com>, David Rowley <dgrowleyml@gmail.com>
Date: 2023-02-21T15:16:33Z
Lists: pgsql-hackers 
On 11/02/2023 23:36, Andres Freund wrote:
> On 2023-02-11 23:03:56 +0200, Heikki Linnakangas wrote:
>> * I don't understand this comment:
>>
>>> 	/*
>>> 	 * Clear out the buffer's tag and flags and usagecount.  We must do
>>> 	 * this to ensure that linear scans of the buffer array don't think
>>> 	 * the buffer is valid.
>>> 	 *
>>> 	 * XXX: This is a pre-existing comment I just moved, but isn't it
>>> 	 * entirely bogus with regard to the tag? We can't do anything with
>>> 	 * the buffer without taking BM_VALID / BM_TAG_VALID into
>>> 	 * account. Likely doesn't matter because we're already dirtying the
>>> 	 * cacheline, but still.
>>> 	 *
>>> 	 */
>>> 	ClearBufferTag(&buf_hdr->tag);
>>> 	buf_state &= ~(BUF_FLAG_MASK | BUF_USAGECOUNT_MASK);
>>> 	UnlockBufHdr(buf_hdr, buf_state);
>>
>> What exactly is wrong with clearing the tag? What does dirtying the
>> cacheline have to do with the correctness here?
> 
> There's nothing wrong with clearing out the tag, but I don't think it's a hard
> requirement today, and certainly not for the reason stated above.
> 
> Validity of the buffer isn't determined by the tag, it's determined by
> BM_VALID (or, if you interpret valid more widely, BM_TAG_VALID).
> 
> Without either having pinned the buffer, or holding the buffer header
> spinlock, the tag can change at any time. And code like DropDatabaseBuffers()
> knows that, and re-checks the the tag after locking the buffer header
> spinlock.
> 
> Afaict, there'd be no correctness issue with removing the
> ClearBufferTag(). There would be an efficiency issue though, because when
> encountering an invalid buffer, we'd unnecessarily enter InvalidateBuffer(),
> which'd find that BM_[TAG_]VALID isn't set, and not to anything.

Okay, gotcha.

> Even though it's not a correctness issue, it seems to me that
> DropRelationsAllBuffers() etc ought to check if the buffer is BM_TAG_VALID,
> before doing anything further.  Particularly in DropRelationsAllBuffers(), the
> check we do for each buffer isn't cheap. Doing it for buffers that don't even
> have a tag seems .. not smart.

Depends on what percentage of buffers are valid, I guess. If all buffers 
are valid, checking BM_TAG_VALID first would lose. In practice, I doubt 
it makes any measurable difference either way.

Since we're micro-optimizing, I noticed that 
BufTagMatchesRelFileLocator() compares the fields in order "spcOid, 
dbOid, relNumber". Before commit 82ac34db20, we used 
RelFileLocatorEqual(), which has this comment:

/*
  * Note: RelFileLocatorEquals and RelFileLocatorBackendEquals compare 
relNumber
  * first since that is most likely to be different in two unequal
  * RelFileLocators.  It is probably redundant to compare spcOid if the 
other
  * fields are found equal, but do it anyway to be sure.  Likewise for 
checking
  * the backend ID in RelFileLocatorBackendEquals.
  */

So we lost that micro-optimization. Should we reorder the checks in 
BufTagMatchesRelFileLocator()?

- Heikki

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.