From: Andres Freund <andres@anarazel.de>
To: pgsql-hackers@postgresql.org, Thomas Munro <thomas.munro@gmail.com>
To: Melanie Plageman <melanieplageman@gmail.com>
Cc: Yura Sokolov <y.sokolov@postgrespro.ru>
Cc: Robert Haas <robertmhaas@gmail.com>
Subject: refactoring relation extension and BufferAlloc(), faster COPY
Date: Fri, 28 Oct 2022 19:54:20 -0700
Message-ID: <20221029025420.eplyow6k7tgu6he3@awork3.anarazel.de> (raw)
Hi,
I'm working to extract independently useful bits from my AIO work, to reduce
the size of that patchset. This is one of those pieces.
In workloads that extend relations a lot, we end up being extremely contended
on the relation extension lock. We've attempted to address that to some degree
by using batching, which helps, but only so much.
The fundamental issue, in my opinion, is that we do *way* too much while
holding the relation extension lock. We acquire a victim buffer, if that
buffer is dirty, we potentially flush the WAL, then write out that
buffer. Then we zero out the buffer contents. Call smgrextend().
Most of that work does not actually need to happen while holding the relation
extension lock. As far as I can tell, the minimum that needs to be covered by
the extension lock is the following:
1) call smgrnblocks()
2) insert buffer[s] into the buffer mapping table at the location returned by
smgrnblocks
3) mark buffer[s] as IO_IN_PROGRESS
1) obviously has to happen with the relation extension lock held because
otherwise we might miss another relation extension. 2+3) need to happen with
the lock held, because otherwise another backend not doing an extension could
read the block before we're done extending, dirty it, write it out, and then
have it overwritten by the extending backend.
The reason we currently do so much work while holding the relation extension
lock is that bufmgr.c does not know about the relation lock and that relation
extension happens entirely within ReadBuffer* - there's no way to use a
narrower scope for the lock.
My fix for that is to add a dedicated function for extending relations, that
can acquire the extension lock if necessary (callers can tell it to skip that,
e.g., when initially creating an init fork). This routine is called by
ReadBuffer_common() when P_NEW is passed in, to provide backward
compatibility.
To be able to acquire victim buffers outside of the extension lock, victim
buffers are now acquired separately from inserting the new buffer mapping
entry. Victim buffer are pinned, cleaned, removed from the buffer mapping
table and marked invalid. Because they are pinned, clock sweeps in other
backends won't return them. This is done in a new function,
[Local]BufferAlloc().
This is similar to Yuri's patch at [0], but not that similar to earlier or
later approaches in that thread. I don't really understand why that thread
went on to ever more complicated approaches, when the basic approach shows
plenty gains, with no issues around the number of buffer mapping entries that
can exist etc.
Other interesting bits I found:
a) For workloads that [mostly] fit into s_b, the smgwrite() that BufferAlloc()
does, nearly doubles the amount of writes. First the kernel ends up writing
out all the zeroed out buffers after a while, then when we write out the
actual buffer contents.
The best fix for that seems to be to optionally use posix_fallocate() to
reserve space, without dirtying pages in the kernel page cache. However, it
looks like that's only beneficial when extending by multiple pages at once,
because it ends up causing one filesystem-journal entry for each extension
on at least some filesystems.
I added 'smgrzeroextend()' that can extend by multiple blocks, without the
caller providing a buffer to write out. When extending by 8 or more blocks,
posix_fallocate() is used if available, otherwise pg_pwritev_with_retry() is
used to extend the file.
b) I found that is quite beneficial to bulk-extend the relation with
smgrextend() even without concurrency. The reason for that is the primarily
the aforementioned dirty buffers that our current extension method causes.
One bit that stumped me for quite a while is to know how much to extend the
relation by. RelationGetBufferForTuple() drives the decision whether / how
much to bulk extend purely on the contention on the extension lock, which
obviously does not work for non-concurrent workloads.
After quite a while I figured out that we actually have good information on
how much to extend by, at least for COPY /
heap_multi_insert(). heap_multi_insert() can compute how much space is
needed to store all tuples, and pass that on to
RelationGetBufferForTuple().
For that to be accurate we need to recompute that number whenever we use an
already partially filled page. That's not great, but doesn't appear to be a
measurable overhead.
c) Contention on the FSM and the pages returned by it is a serious bottleneck
after a) and b).
The biggest issue is that the current bulk insertion logic in hio.c enters
all but one of the new pages into the freespacemap. That will immediately
cause all the other backends to contend on the first few pages returned the
FSM, and cause contention on the FSM pages itself.
I've, partially, addressed that by using the information about the required
number of pages from b). Whether we bulk insert or not, the number of pages
we know we're going to need for one heap_multi_insert() don't need to be
added to the FSM - we're going to use them anyway.
I've stashed the number of free blocks in the BulkInsertState for now, but
I'm not convinced that that's the right place.
If I revert just this part, the "concurrent COPY into unlogged table"
benchmark goes from ~240 tps to ~190 tps.
Even after that change the FSM is a major bottleneck. Below I included
benchmarks showing this by just removing the use of the FSM, but I haven't
done anything further about it. The contention seems to be both from
updating the FSM, as well as thundering-herd like symptoms from accessing
the FSM.
The update part could likely be addressed to some degree with a batch
update operation updating the state for multiple pages.
The access part could perhaps be addressed by adding an operation that gets
a page and immediately marks it as fully used, so other backends won't also
try to access it.
d) doing
/* new buffers are zero-filled */
MemSet((char *) bufBlock, 0, BLCKSZ);
under the extension lock is surprisingly expensive on my two socket
workstation (but much less noticable on my laptop).
If I move the MemSet back under the extension lock, the "concurrent COPY
into unlogged table" benchmark goes from ~240 tps to ~200 tps.
e) When running a few benchmarks for this email, I noticed that there was a
sharp performance dropoff for the patched code for a pgbench -S -s100 on a
database with 1GB s_b, start between 512 and 1024 clients. This started with
the patch only acquiring one buffer partition lock at a time. Lots of
debugging ensued, resulting in [3].
The problem isn't actually related to the change, it just makes it more
visible, because the "lock chains" between two partitions reduce the
average length of the wait queues substantially, by distribution them
between more partitions. [3] has a reproducer that's entirely independent
of this patchset.
Bulk extension acquires a number of victim buffers, acquires the extension
lock, inserts the buffers into the buffer mapping table and marks them as
io-in-progress, calls smgrextend and releases the extension lock. After that
buffer[s] are locked (depending on mode and an argument indicating the number
of blocks to be locked), and TerminateBufferIO() is called.
This requires two new pieces of infrastructure:
First, pinning multiple buffers opens up the obvious danger that we might run
of non-pinned buffers. I added LimitAdditional[Local]Pins() that allows each
backend to pin a proportional share of buffers (although always allowing one,
as we do today).
Second, having multiple IOs in progress at the same time isn't possible with
the InProgressBuf mechanism. I added a ResourceOwnerRememberBufferIO() etc to
deal with that instead. I like that this ends up removing a lot of
AbortBufferIO() calls from the loops of various aux processes (now released
inside ReleaseAuxProcessResources()).
In very extreme workloads (single backend doing a pgbench -S -s 100 against a
s_b=64MB database) the memory allocations triggered by StartBufferIO() are
*just about* visible, not sure if that's worth worrying about - we do such
allocations for the much more common pinning of buffers as well.
The new [Bulk]ExtendSharedRelationBuffered() currently have both a Relation
and a SMgrRelation argument, requiring at least one of them to be set. The
reason for that is on the one hand that LockRelationForExtension() requires a
relation and on the other hand, redo routines typically don't have a Relation
around (recovery doesn't require an extension lock). That's not pretty, but
seems a tad better than the ReadBufferExtended() vs
ReadBufferWithoutRelcache() mess.
I've done a fair bit of benchmarking of this patchset. For COPY it comes out
ahead everywhere. It's possible that there's a very small regression for
extremly IO miss heavy workloads, more below.
server "base" configuration:
max_wal_size=150GB
shared_buffers=24GB
huge_pages=on
autovacuum=0
backend_flush_after=2MB
max_connections=5000
wal_buffers=128MB
wal_segment_size=1GB
benchmark: pgbench running COPY into a single table. pgbench -t is set
according to the client count, so that the same amount of data is inserted.
This is done oth using small files ([1], ringbuffer not effective, no dirty
data to write out within the benchmark window) and a bit larger files ([2],
lots of data to write out due to ringbuffer).
To make it a fair comparison HEAD includes the lwlock-waitqueue fix as well.
s_b=24GB
test: unlogged_small_files, format: text, files: 1024, 9015MB total
seconds tbl-MBs seconds tbl-MBs seconds tbl-MBs
clients HEAD HEAD patch patch no_fsm no_fsm
1 58.63 207 50.22 242 54.35 224
2 32.67 372 25.82 472 27.30 446
4 22.53 540 13.30 916 14.33 851
8 15.14 804 7.43 1640 7.48 1632
16 14.69 829 4.79 2544 4.50 2718
32 15.28 797 4.41 2763 3.32 3710
64 15.34 794 5.22 2334 3.06 4061
128 15.49 786 4.97 2452 3.13 3926
256 15.85 768 5.02 2427 3.26 3769
512 16.02 760 5.29 2303 3.54 3471
test: logged_small_files, format: text, files: 1024, 9018MB total
seconds tbl-MBs seconds tbl-MBs seconds tbl-MBs
clients HEAD HEAD patch patch no_fsm no_fsm
1 68.18 178 59.41 205 63.43 192
2 39.71 306 33.10 368 34.99 348
4 27.26 446 19.75 617 20.09 607
8 18.84 646 12.86 947 12.68 962
16 15.96 763 9.62 1266 8.51 1436
32 15.43 789 8.20 1486 7.77 1579
64 16.11 756 8.91 1367 8.90 1383
128 16.41 742 10.00 1218 9.74 1269
256 17.33 702 11.91 1023 10.89 1136
512 18.46 659 14.07 866 11.82 1049
test: unlogged_medium_files, format: text, files: 64, 9018MB total
seconds tbl-MBs seconds tbl-MBs seconds tbl-MBs
clients HEAD HEAD patch patch no_fsm no_fsm
1 63.27s 192 56.14 217 59.25 205
2 40.17s 303 29.88 407 31.50 386
4 27.57s 442 16.16 754 17.18 709
8 21.26s 573 11.89 1025 11.09 1099
16 21.25s 573 10.68 1141 10.22 1192
32 21.00s 580 10.72 1136 10.35 1178
64 20.64s 590 10.15 1200 9.76 1249
128 skipped
256 skipped
512 skipped
test: logged_medium_files, format: text, files: 64, 9018MB total
seconds tbl-MBs seconds tbl-MBs seconds tbl-MBs
clients HEAD HEAD patch patch no_fsm no_fsm
1 71.89s 169 65.57 217 69.09 69.09
2 47.36s 257 36.22 407 38.71 38.71
4 33.10s 368 21.76 754 22.78 22.78
8 26.62s 457 15.89 1025 15.30 15.30
16 24.89s 489 17.08 1141 15.20 15.20
32 25.15s 484 17.41 1136 16.14 16.14
64 26.11s 466 17.89 1200 16.76 16.76
128 skipped
256 skipped
512 skipped
Just to see how far it can be pushed, with binary format we can now get to
nearly 6GB/s into a table when disabling the FSM - note the 2x difference
between patch and patch+no-fsm at 32 clients.
test: unlogged_small_files, format: binary, files: 1024, 9508MB total
seconds tbl-MBs seconds tbl-MBs seconds tbl-MBs
clients HEAD HEAD patch patch no_fsm no_fsm
1 34.14 357 28.04 434 29.46 413
2 22.67 537 14.42 845 14.75 826
4 16.63 732 7.62 1599 7.69 1587
8 13.48 904 4.36 2795 4.13 2959
16 14.37 848 3.78 3224 2.74 4493
32 14.79 823 4.20 2902 2.07 5974
64 14.76 825 5.03 2423 2.21 5561
128 14.95 815 4.36 2796 2.30 5343
256 15.18 802 4.31 2828 2.49 4935
512 15.41 790 4.59 2656 2.84 4327
s_b=4GB
test: unlogged_small_files, format: text, files: 1024, 9018MB total
seconds tbl-MBs seconds tbl-MBs
clients HEAD HEAD patch patch
1 62.55 194 54.22 224
2 37.11 328 28.94 421
4 25.97 469 16.42 742
8 20.01 609 11.92 1022
16 19.55 623 11.05 1102
32 19.34 630 11.27 1081
64 19.07 639 12.04 1012
128 19.22 634 12.27 993
256 19.34 630 12.28 992
512 19.60 621 11.74 1038
test: logged_small_files, format: text, files: 1024, 9018MB total
seconds tbl-MBs seconds tbl-MBs
clients HEAD HEAD patch patch
1 71.71 169 63.63 191
2 46.93 259 36.31 335
4 30.37 401 22.41 543
8 22.86 533 16.90 721
16 20.18 604 14.07 866
32 19.64 620 13.06 933
64 19.71 618 15.08 808
128 19.95 610 15.47 787
256 20.48 595 16.53 737
512 21.56 565 16.86 722
test: unlogged_medium_files, format: text, files: 64, 9018MB total
seconds tbl-MBs seconds tbl-MBs
clients HEAD HEAD patch patch
1 62.65 194 55.74 218
2 40.25 302 29.45 413
4 27.37 445 16.26 749
8 22.07 552 11.75 1037
16 21.29 572 10.64 1145
32 20.98 580 10.70 1139
64 20.65 590 10.21 1193
128 skipped
256 skipped
512 skipped
test: logged_medium_files, format: text, files: 64, 9018MB total
seconds tbl-MBs seconds tbl-MBs
clients HEAD HEAD patch patch
1 71.72 169 65.12 187
2 46.46 262 35.74 341
4 32.61 373 21.60 564
8 26.69 456 16.30 747
16 25.31 481 17.00 716
32 24.96 488 17.47 697
64 26.05 467 17.90 680
128 skipped
256 skipped
512 skipped
test: unlogged_small_files, format: binary, files: 1024, 9505MB total
seconds tbl-MBs seconds tbl-MBs
clients HEAD HEAD patch patch
1 37.62 323 32.77 371
2 28.35 429 18.89 645
4 20.87 583 12.18 1000
8 19.37 629 10.38 1173
16 19.41 627 10.36 1176
32 18.62 654 11.04 1103
64 18.33 664 11.89 1024
128 18.41 661 11.91 1023
256 18.52 658 12.10 1007
512 18.78 648 11.49 1060
benchmark: Run a pgbench -S workload with scale 100, so it doesn't fit into
s_b, thereby exercising BufferAlloc()'s buffer replacement path heavily.
The run-to-run variance on my workstation is high for this workload (both
before/after my changes). I also found that the ramp-up time at higher client
counts is very significant:
progress: 2.1 s, 5816.8 tps, lat 1.835 ms stddev 4.450, 0 failed
progress: 3.0 s, 666729.4 tps, lat 5.755 ms stddev 16.753, 0 failed
progress: 4.0 s, 899260.1 tps, lat 3.619 ms stddev 41.108, 0 failed
...
One would need to run pgbench for impractically long to make that effect
vanish.
My not great solution for these was to run with -T21 -P5 and use the best 5s
as the tps.
s_b=1GB
tps tps
clients master patched
1 49541 48805
2 85342 90010
4 167340 168918
8 308194 303222
16 524294 523678
32 649516 649100
64 932547 937702
128 908249 906281
256 856496 903979
512 764254 934702
1024 653886 925113
2048 569695 917262
4096 526782 903258
s_b=128MB:
tps tps
clients master patched
1 40407 39854
2 73180 72252
4 143334 140860
8 240982 245331
16 429265 420810
32 544593 540127
64 706408 726678
128 713142 718087
256 611030 695582
512 552751 686290
1024 508248 666370
2048 474108 656735
4096 448582 633040
As there might be a small regression at the smallest end, I ran a more extreme
version of the above. Using a pipelined pgbench -S, with a single client, for
longer. With s_b=8MB.
To further reduce noise I pinned the server to one cpu, the client to another
and disabled turbo mode on the CPU.
master "total" tps: 61.52
master "best 5s" tps: 61.8
patch "total" tps: 61.20
patch "best 5s" tps: 61.4
Hardly conclusive, but it does look like there's a small effect. It could be
code layout or such.
My guess however is that it's the resource owner for in-progress IO that I
added - that adds an additional allocation inside the resowner machinery. I
commented those out (that's obviously incorrect!) just to see whether that
changes anything:
no-resowner "total" tps: 62.03
no-resowner "best 5s" tps: 62.2
So it looks like indeed, it's the resowner. I am a bit surprised, because
obviously we already use that mechanism for pins, which obviously is more
frequent.
I'm not sure it's worth worrying about - this is a pretty absurd workload. But
if we decide it is, I can think of a few ways to address this. E.g.:
- We could preallocate an initial element inside the ResourceArray struct, so
that a newly created resowner won't need to allocate immediately
- We could only use resowners if there's more than one IO in progress at the
same time - but I don't like that idea much
- We could try to store the "in-progress"-ness of a buffer inside the 'bufferpin'
resowner entry - on 64bit system there's plenty space for that. But on 32bit systems...
The patches here aren't fully polished (as will be evident). But they should
be more than good enough to discuss whether this is a sane direction.
Greetings,
Andres Freund
[0] https://postgr.es/m/3b108afd19fa52ed20c464a69f64d545e4a14772.camel%40postgrespro.ru
[1] COPY (SELECT repeat(random()::text, 5) FROM generate_series(1, 100000)) TO '/tmp/copytest_data_text.copy' WITH (FORMAT test);
[2] COPY (SELECT repeat(random()::text, 5) FROM generate_series(1, 6*100000)) TO '/tmp/copytest_data_text.copy' WITH (FORMAT text);
[3] https://postgr.es/m/20221027165914.2hofzp4cvutj6gin@awork3.anarazel.deAttachments:
[text/x-diff] v1-0001-wip-lwlock-fix-quadratic-behaviour-with-very-long.patch (3.8K, ../20221029025420.eplyow6k7tgu6he3@awork3.anarazel.de/2-v1-0001-wip-lwlock-fix-quadratic-behaviour-with-very-long.patch)
download | inline diff:
From 57306a6a2a4e7300206d3a5e40a1787b427765b8 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Thu, 27 Oct 2022 10:18:13 -0700
Subject: [PATCH v1 01/12] wip: lwlock: fix quadratic behaviour with very long
wait lists
Author:
Reviewed-by:
Discussion: https://postgr.es/m/
Backpatch:
---
src/include/storage/proc.h | 3 ++-
src/backend/storage/lmgr/lwlock.c | 29 +++++++++++++++--------------
2 files changed, 17 insertions(+), 15 deletions(-)
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.hindex 8d096fdeeb1..9a2615666a1 100644--- a/src/include/storage/proc.h+++ b/src/include/storage/proc.h@@ -217,7 +217,8 @@ struct PGPROC
bool recoveryConflictPending;
/* Info about LWLock the process is currently waiting for, if any. */
- bool lwWaiting; /* true if waiting for an LW lock */+ int lwWaiting; /* 0 if not waiting, 1 if on waitlist, 2 if+ * waiting to be woken */
uint8 lwWaitMode; /* lwlock mode being waited for */
proclist_node lwWaitLink; /* position in LW lock wait list */
diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.cindex d274c9b1dc9..2d1c8fae06e 100644--- a/src/backend/storage/lmgr/lwlock.c+++ b/src/backend/storage/lmgr/lwlock.c@@ -987,6 +987,9 @@ LWLockWakeup(LWLock *lock)
wokeup_somebody = true;
}
+ /* signal that the process isn't on the wait list anymore */+ waiter->lwWaiting = 2;+
/*
* Once we've woken up an exclusive lock, there's no point in waking
* up anybody else.
@@ -1044,7 +1047,7 @@ LWLockWakeup(LWLock *lock)
* another lock.
*/
pg_write_barrier();
- waiter->lwWaiting = false;+ waiter->lwWaiting = 0;
PGSemaphoreUnlock(waiter->sem);
}
}
@@ -1073,7 +1076,7 @@ LWLockQueueSelf(LWLock *lock, LWLockMode mode)
/* setting the flag is protected by the spinlock */
pg_atomic_fetch_or_u32(&lock->state, LW_FLAG_HAS_WAITERS);
- MyProc->lwWaiting = true;+ MyProc->lwWaiting = 1;
MyProc->lwWaitMode = mode;
/* LW_WAIT_UNTIL_FREE waiters are always at the front of the queue */
@@ -1101,7 +1104,6 @@ static void
LWLockDequeueSelf(LWLock *lock)
{
bool found = false;
- proclist_mutable_iter iter;
#ifdef LWLOCK_STATS
lwlock_stats *lwstats;
@@ -1114,17 +1116,14 @@ LWLockDequeueSelf(LWLock *lock)
LWLockWaitListLock(lock);
/*
- * Can't just remove ourselves from the list, but we need to iterate over- * all entries as somebody else could have dequeued us.+ * Remove ourselves from the waitlist, unless we've already been+ * removed. The removal happens with the wait list lock held, so there's+ * no race in this check.
*/
- proclist_foreach_modify(iter, &lock->waiters, lwWaitLink)+ if (MyProc->lwWaiting == 1)
{
- if (iter.cur == MyProc->pgprocno)- {- found = true;- proclist_delete(&lock->waiters, iter.cur, lwWaitLink);- break;- }+ proclist_delete(&lock->waiters, MyProc->pgprocno, lwWaitLink);+ found = true;
}
if (proclist_is_empty(&lock->waiters) &&
@@ -1138,7 +1137,7 @@ LWLockDequeueSelf(LWLock *lock)
/* clear waiting state again, nice for debugging */
if (found)
- MyProc->lwWaiting = false;+ MyProc->lwWaiting = 0;
else
{
int extraWaits = 0;
@@ -1772,6 +1771,8 @@ LWLockUpdateVar(LWLock *lock, uint64 *valptr, uint64 val)
proclist_delete(&lock->waiters, iter.cur, lwWaitLink);
proclist_push_tail(&wakeup, iter.cur, lwWaitLink);
++ waiter->lwWaiting = 2;
}
/* We are done updating shared state of the lock itself. */
@@ -1787,7 +1788,7 @@ LWLockUpdateVar(LWLock *lock, uint64 *valptr, uint64 val)
proclist_delete(&wakeup, iter.cur, lwWaitLink);
/* check comment in LWLockWakeup() about this barrier */
pg_write_barrier();
- waiter->lwWaiting = false;+ waiter->lwWaiting = 0;
PGSemaphoreUnlock(waiter->sem);
}
}
--
2.38.0
[text/x-diff] v1-0002-aio-Add-some-error-checking-around-pinning.patch (3.1K, ../20221029025420.eplyow6k7tgu6he3@awork3.anarazel.de/3-v1-0002-aio-Add-some-error-checking-around-pinning.patch)
download | inline diff:
From 58ffcee444a3b28b37cd7458e179f891fe1578ae Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Wed, 1 Jul 2020 19:06:45 -0700
Subject: [PATCH v1 02/12] aio: Add some error checking around pinning.
---
src/include/storage/bufmgr.h | 1 +
src/backend/storage/buffer/bufmgr.c | 42 ++++++++++++++++++++---------
2 files changed, 30 insertions(+), 13 deletions(-)
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.hindex 6f4dfa09602..2c1f12d3066 100644--- a/src/include/storage/bufmgr.h+++ b/src/include/storage/bufmgr.h@@ -120,6 +120,7 @@ extern void ReleaseBuffer(Buffer buffer);
extern void UnlockReleaseBuffer(Buffer buffer);
extern void MarkBufferDirty(Buffer buffer);
extern void IncrBufferRefCount(Buffer buffer);
+extern void BufferCheckOneLocalPin(Buffer buffer);
extern Buffer ReleaseAndReadBuffer(Buffer buffer, Relation relation,
BlockNumber blockNum);
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.cindex 6b953814812..dfb7cfe0ef3 100644--- a/src/backend/storage/buffer/bufmgr.c+++ b/src/backend/storage/buffer/bufmgr.c@@ -1707,6 +1707,8 @@ PinBuffer(BufferDesc *buf, BufferAccessStrategy strategy)
bool result;
PrivateRefCountEntry *ref;
+ Assert(!BufferIsLocal(b));+
ref = GetPrivateRefCountEntry(b, true);
if (ref == NULL)
@@ -1852,6 +1854,8 @@ UnpinBuffer(BufferDesc *buf)
PrivateRefCountEntry *ref;
Buffer b = BufferDescriptorGetBuffer(buf);
+ Assert(!BufferIsLocal(b));+
/* not moving as we're likely deleting it soon anyway */
ref = GetPrivateRefCountEntry(b, false);
Assert(ref != NULL);
@@ -4209,6 +4213,25 @@ ConditionalLockBuffer(Buffer buffer)
LW_EXCLUSIVE);
}
+void+BufferCheckOneLocalPin(Buffer buffer)+{+ if (BufferIsLocal(buffer))+ {+ /* There should be exactly one pin */+ if (LocalRefCount[-buffer - 1] != 1)+ elog(ERROR, "incorrect local pin count: %d",+ LocalRefCount[-buffer - 1]);+ }+ else+ {+ /* There should be exactly one local pin */+ if (GetPrivateRefCount(buffer) != 1)+ elog(ERROR, "incorrect local pin count: %d",+ GetPrivateRefCount(buffer));+ }+}+
/*
* LockBufferForCleanup - lock a buffer in preparation for deleting items
*
@@ -4236,20 +4259,11 @@ LockBufferForCleanup(Buffer buffer)
Assert(BufferIsPinned(buffer));
Assert(PinCountWaitBuf == NULL);
- if (BufferIsLocal(buffer))- {- /* There should be exactly one pin */- if (LocalRefCount[-buffer - 1] != 1)- elog(ERROR, "incorrect local pin count: %d",- LocalRefCount[-buffer - 1]);- /* Nobody else to wait for */- return;- }+ BufferCheckOneLocalPin(buffer);- /* There should be exactly one local pin */- if (GetPrivateRefCount(buffer) != 1)- elog(ERROR, "incorrect local pin count: %d",- GetPrivateRefCount(buffer));+ /* Nobody else to wait for */+ if (BufferIsLocal(buffer))+ return;
bufHdr = GetBufferDescriptor(buffer - 1);
@@ -4757,6 +4771,8 @@ LockBufHdr(BufferDesc *desc)
SpinDelayStatus delayStatus;
uint32 old_buf_state;
+ Assert(!BufferIsLocal(BufferDescriptorGetBuffer(desc)));+
init_local_spin_delay(&delayStatus);
while (true)
--
2.38.0
[text/x-diff] v1-0003-hio-Release-extension-lock-before-initializing-pa.patch (2.0K, ../20221029025420.eplyow6k7tgu6he3@awork3.anarazel.de/4-v1-0003-hio-Release-extension-lock-before-initializing-pa.patch)
download | inline diff:
From 0ee5910af08188ff894fb4f06bbe7907f9d7da86 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Mon, 24 Oct 2022 12:28:06 -0700
Subject: [PATCH v1 03/12] hio: Release extension lock before initializing page
/ pinning VM
PageInit() while holding the extension lock is unnecessary after 0d1fe9f74e3
started to use RBM_ZERO_AND_LOCK - nobody can look at the new page before we
release the page lock. PageInit() zeroes the page, which isn't that cheap, so
deferring it until after the extension lock is released seems like a good idea.
Doing visibilitymap_pin() while holding the extension lock, introduced in
7db0cd2145f2, looks like an accident. Due to the restrictions on
HEAP_INSERT_FROZEN it's unlikely to be a performance issue, but it still seems
better to move it out.
---
src/backend/access/heap/hio.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.cindex b0ece666293..bc4035a6221 100644--- a/src/backend/access/heap/hio.c+++ b/src/backend/access/heap/hio.c@@ -623,6 +623,13 @@ loop:
*/
buffer = ReadBufferBI(relation, P_NEW, RBM_ZERO_AND_LOCK, bistate);
+ /*+ * Release the file-extension lock; it's now OK for someone else to extend+ * the relation some more.+ */+ if (needLock)+ UnlockRelationForExtension(relation, ExclusiveLock);+
/*
* We need to initialize the empty new page. Double-check that it really
* is empty (this should never happen, but if it does we don't want to
@@ -647,13 +654,6 @@ loop:
visibilitymap_pin(relation, BufferGetBlockNumber(buffer), vmbuffer);
}
- /*- * Release the file-extension lock; it's now OK for someone else to extend- * the relation some more.- */- if (needLock)- UnlockRelationForExtension(relation, ExclusiveLock);-
/*
* Lock the other buffer. It's guaranteed to be of a lower page number
* than the new page. To conform with the deadlock prevent rules, we ought
--
2.38.0
[text/x-diff] v1-0004-Add-smgrzeroextend-FileZero-FileFallocate.patch (12.0K, ../20221029025420.eplyow6k7tgu6he3@awork3.anarazel.de/5-v1-0004-Add-smgrzeroextend-FileZero-FileFallocate.patch)
download | inline diff:
From ad8d317335092201682f9759cf88ee6382918f7d Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Sun, 23 Oct 2022 14:25:46 -0700
Subject: [PATCH v1 04/12] Add smgrzeroextend(), FileZero(), FileFallocate()
smgrzeroextend() uses FileFallocate() to efficiently extend files by multiple
blocks. When extending by a small number of blocks, use FileZero() instead, as
using posix_fallocate() for small numbers of blocks is inefficient for some
file systems / operating systems. FileZero() is also used as the fallback for
FileFallocate() on platforms / filesystems that don't support fallocate.
Author:
Reviewed-by:
Discussion: https://postgr.es/m/
Backpatch:
---
src/include/storage/fd.h | 3 +
src/include/storage/md.h | 2 +
src/include/storage/smgr.h | 2 +
src/backend/storage/file/fd.c | 105 ++++++++++++++++++++++++++++++++
src/backend/storage/smgr/md.c | 103 +++++++++++++++++++++++++++++++
src/backend/storage/smgr/smgr.c | 21 +++++++
6 files changed, 236 insertions(+)
diff --git a/src/include/storage/fd.h b/src/include/storage/fd.hindex c0a212487d9..3d309edb152 100644--- a/src/include/storage/fd.h+++ b/src/include/storage/fd.h@@ -106,6 +106,9 @@ extern int FilePrefetch(File file, off_t offset, int amount, uint32 wait_event_i
extern int FileRead(File file, char *buffer, int amount, off_t offset, uint32 wait_event_info);
extern int FileWrite(File file, char *buffer, int amount, off_t offset, uint32 wait_event_info);
extern int FileSync(File file, uint32 wait_event_info);
+extern int FileZero(File file, off_t offset, off_t len, uint32 wait_event_info);+extern int FileFallocate(File file, off_t offset, off_t len, uint32 wait_event_info);+
extern off_t FileSize(File file);
extern int FileTruncate(File file, off_t offset, uint32 wait_event_info);
extern void FileWriteback(File file, off_t offset, off_t nbytes, uint32 wait_event_info);
diff --git a/src/include/storage/md.h b/src/include/storage/md.hindex 10aa1b0109b..d0597f8a575 100644--- a/src/include/storage/md.h+++ b/src/include/storage/md.h@@ -28,6 +28,8 @@ extern bool mdexists(SMgrRelation reln, ForkNumber forknum);
extern void mdunlink(RelFileLocatorBackend rlocator, ForkNumber forknum, bool isRedo);
extern void mdextend(SMgrRelation reln, ForkNumber forknum,
BlockNumber blocknum, char *buffer, bool skipFsync);
+extern void mdzeroextend(SMgrRelation reln, ForkNumber forknum,+ BlockNumber blocknum, int nblocks, bool skipFsync);
extern bool mdprefetch(SMgrRelation reln, ForkNumber forknum,
BlockNumber blocknum);
extern void mdread(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
diff --git a/src/include/storage/smgr.h b/src/include/storage/smgr.hindex a07715356ba..503310e82ba 100644--- a/src/include/storage/smgr.h+++ b/src/include/storage/smgr.h@@ -92,6 +92,8 @@ extern void smgrdosyncall(SMgrRelation *rels, int nrels);
extern void smgrdounlinkall(SMgrRelation *rels, int nrels, bool isRedo);
extern void smgrextend(SMgrRelation reln, ForkNumber forknum,
BlockNumber blocknum, char *buffer, bool skipFsync);
+extern void smgrzeroextend(SMgrRelation reln, ForkNumber forknum,+ BlockNumber blocknum, int nblocks, bool skipFsync);
extern bool smgrprefetch(SMgrRelation reln, ForkNumber forknum,
BlockNumber blocknum);
extern void smgrread(SMgrRelation reln, ForkNumber forknum,
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.cindex 4151cafec54..47b620649aa 100644--- a/src/backend/storage/file/fd.c+++ b/src/backend/storage/file/fd.c@@ -93,6 +93,7 @@
#include "common/pg_prng.h"
#include "miscadmin.h"
#include "pgstat.h"
+#include "port/pg_iovec.h"
#include "portability/mem.h"
#include "postmaster/startup.h"
#include "storage/fd.h"
@@ -2205,6 +2206,105 @@ FileSync(File file, uint32 wait_event_info)
return returnCode;
}
+/* So that FileZero() doesn't have to re-zero a block on every call */+static const PGAlignedBlock zerobuf = {0};++int+FileZero(File file, off_t offset, off_t len, uint32 wait_event_info)+{+ int returnCode;+ int numblocks;+ struct iovec iov[PG_IOV_MAX];++ /*+ * FIXME: Quick-and-dirty implementation, to be replaced by+ * pg_pwrite_zeros() from+ * https://postgr.es/m/Y1oc%2BFjiyVjNZa%2BL%40paquier.xyz+ *+ * Otherwise it'd not at all be ok to rely on len being a multiple of+ * BLCKSZ.+ */+ Assert((len % BLCKSZ) == 0);++ Assert(FileIsValid(file));+ returnCode = FileAccess(file);+ if (returnCode < 0)+ return returnCode;++ numblocks = len / BLCKSZ;++ for (int i = 0; i < Min(numblocks, lengthof(iov)); ++i)+ {+ iov[i].iov_base = (char *) zerobuf.data;+ iov[i].iov_len = BLCKSZ;+ }++ while (numblocks > 0)+ {+ int iovcnt = Min(numblocks, lengthof(iov));+ off_t seekpos_l = offset;+ ssize_t ret;++ pgstat_report_wait_start(wait_event_info);+ ret = pg_pwritev_with_retry(VfdCache[file].fd, iov, iovcnt, seekpos_l);+ pgstat_report_wait_end();++ if (ret < 0)+ return -1;++ Assert(ret == iovcnt * BLCKSZ);+ offset += iovcnt * BLCKSZ;+ numblocks -= iovcnt;+ }++ return 0;+}++/*+ * Try to reserve file space with posix_fallocate(). If posix_fallocate() is+ * not implemented on the operating system or fails with EINVAL / EOPNOTSUPP,+ * use FileZero() instead.+ *+ * Note that at least glibc() implements posix_fallocate() in userspace if not+ * implemented by the filesystem. That's not the case for all environments+ * though.+ */+int+FileFallocate(File file, off_t offset, off_t len, uint32 wait_event_info)+{+ int returnCode;++ Assert(FileIsValid(file));+ returnCode = FileAccess(file);+ if (returnCode < 0)+ return returnCode;++#ifdef HAVE_POSIX_FALLOCATE+ pgstat_report_wait_start(wait_event_info);+ returnCode = posix_fallocate(VfdCache[file].fd, offset, len);+ pgstat_report_wait_end();++ if (returnCode == 0)+ return 0;++ /* for compatibility with %m printing etc */+ errno = returnCode;++ /*+ * 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;++ if (returnCode == 0 ||+ (returnCode != EINVAL && returnCode != EINVAL))+ return returnCode;+#endif++ return FileZero(file, offset, len, wait_event_info);+}+
off_t
FileSize(File file)
{
@@ -2277,6 +2377,11 @@ int
FileGetRawDesc(File file)
{
Assert(FileIsValid(file));
++ if (FileAccess(file) < 0)+ return -1;++ FileAccess(file);
return VfdCache[file].fd;
}
diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.cindex a515bb36ac1..eee3cae7c4e 100644--- a/src/backend/storage/smgr/md.c+++ b/src/backend/storage/smgr/md.c@@ -28,6 +28,7 @@
#include "access/xlog.h"
#include "access/xlogutils.h"
#include "commands/tablespace.h"
+#include "common/file_utils.h"
#include "miscadmin.h"
#include "pg_trace.h"
#include "pgstat.h"
@@ -486,6 +487,108 @@ mdextend(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
Assert(_mdnblocks(reln, forknum, v) <= ((BlockNumber) RELSEG_SIZE));
}
+void+mdzeroextend(SMgrRelation reln, ForkNumber forknum,+ BlockNumber blocknum, int nblocks, bool skipFsync)+{+ MdfdVec *v;+ BlockNumber curblocknum = blocknum;+ int remblocks = nblocks;++ Assert(nblocks > 0);++ /* This assert is too expensive to have on normally ... */+#ifdef CHECK_WRITE_VS_EXTEND+ Assert(blocknum >= mdnblocks(reln, forknum));+#endif++ /*+ * If a relation manages to grow to 2^32-1 blocks, refuse to extend it any+ * more --- we mustn't create a block whose number actually is+ * InvalidBlockNumber or larger.+ */+ if ((uint64) blocknum + nblocks >= (uint64) InvalidBlockNumber)+ ereport(ERROR,+ (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),+ errmsg("cannot extend file \"%s\" beyond %u blocks",+ relpath(reln->smgr_rlocator, forknum),+ InvalidBlockNumber)));++ while (remblocks > 0)+ {+ int segstartblock = curblocknum % ((BlockNumber) RELSEG_SIZE);+ int segendblock = (curblocknum % ((BlockNumber) RELSEG_SIZE)) + remblocks;+ off_t seekpos = (off_t) BLCKSZ * segstartblock;+ int numblocks;++ if (segendblock > RELSEG_SIZE)+ segendblock = RELSEG_SIZE;++ numblocks = segendblock - segstartblock;++ v = _mdfd_getseg(reln, forknum, curblocknum, skipFsync, EXTENSION_CREATE);++ Assert(segstartblock < RELSEG_SIZE);+ Assert(segendblock <= RELSEG_SIZE);++ /*+ * If available use posix_fallocate() to extend the relation. That's+ * often more efficient than using write(), as it commonly won't cause+ * the kernel to allocate page cache space for the extended pages.+ *+ * However, we shouldn't use fallocate() for small extensions, 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.+ */+ if (numblocks > 8)+ {+ int ret;++ ret = FileFallocate(v->mdfd_vfd, seekpos,+ (off_t) BLCKSZ * numblocks,+ WAIT_EVENT_DATA_FILE_EXTEND);+ if (ret != 0)+ {+ ereport(ERROR,+ (errcode_for_file_access(),+ errmsg("could not extend file \"%s\" with posix_fallocate(): %m",+ FilePathName(v->mdfd_vfd)),+ errhint("Check free disk space.")));+ }+ }+ else+ {+ int ret;++ /*+ * Even if we don't have fallocate, we can still extend a bit more+ * efficiently than writing each 8kB block individually.+ * FileZero() uses pg_writev[with_retry] with a single zeroed+ * buffer to avoid needing a zeroed buffer for the whole length of+ * the extension.+ */+ ret = FileZero(v->mdfd_vfd, seekpos,+ (off_t) BLCKSZ * numblocks,+ WAIT_EVENT_DATA_FILE_EXTEND);+ if (ret < 0)+ ereport(ERROR,+ (errcode_for_file_access(),+ errmsg("could not extend file \"%s\": %m",+ FilePathName(v->mdfd_vfd)),+ errhint("Check free disk space.")));+ }++ if (!skipFsync && !SmgrIsTemp(reln))+ register_dirty_segment(reln, forknum, v);++ Assert(_mdnblocks(reln, forknum, v) <= ((BlockNumber) RELSEG_SIZE));++ remblocks -= segendblock - segstartblock;+ curblocknum += segendblock - segstartblock;+ }+}+
/*
* mdopenfork() -- Open one fork of the specified relation.
*
diff --git a/src/backend/storage/smgr/smgr.c b/src/backend/storage/smgr/smgr.cindex c1a5febcbfd..6fb693cb062 100644--- a/src/backend/storage/smgr/smgr.c+++ b/src/backend/storage/smgr/smgr.c@@ -50,6 +50,8 @@ typedef struct f_smgr
bool isRedo);
void (*smgr_extend) (SMgrRelation reln, ForkNumber forknum,
BlockNumber blocknum, char *buffer, bool skipFsync);
+ void (*smgr_zeroextend) (SMgrRelation reln, ForkNumber forknum,+ BlockNumber blocknum, int nblocks, bool skipFsync);
bool (*smgr_prefetch) (SMgrRelation reln, ForkNumber forknum,
BlockNumber blocknum);
void (*smgr_read) (SMgrRelation reln, ForkNumber forknum,
@@ -75,6 +77,7 @@ static const f_smgr smgrsw[] = {
.smgr_exists = mdexists,
.smgr_unlink = mdunlink,
.smgr_extend = mdextend,
+ .smgr_zeroextend = mdzeroextend,
.smgr_prefetch = mdprefetch,
.smgr_read = mdread,
.smgr_write = mdwrite,
@@ -507,6 +510,24 @@ smgrextend(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
reln->smgr_cached_nblocks[forknum] = InvalidBlockNumber;
}
+void+smgrzeroextend(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,+ int nblocks, bool skipFsync)+{+ smgrsw[reln->smgr_which].smgr_zeroextend(reln, forknum, blocknum,+ nblocks, skipFsync);++ /*+ * Normally we expect this to increase nblocks by nblocks, but if the+ * cached value isn't as expected, just invalidate it so the next call+ * asks the kernel.+ */+ if (reln->smgr_cached_nblocks[forknum] == blocknum)+ reln->smgr_cached_nblocks[forknum] = blocknum + nblocks;+ else+ reln->smgr_cached_nblocks[forknum] = InvalidBlockNumber;+}+
/*
* smgrprefetch() -- Initiate asynchronous read of the specified block of a relation.
*
--
2.38.0
[text/x-diff] v1-0005-bufmgr-Add-Pin-UnpinLocalBuffer.patch (5.3K, ../20221029025420.eplyow6k7tgu6he3@awork3.anarazel.de/6-v1-0005-bufmgr-Add-Pin-UnpinLocalBuffer.patch)
download | inline diff:
From a8804720e75a2cc8828091ea9eec967a2658fc3f Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Wed, 26 Oct 2022 12:05:07 -0700
Subject: [PATCH v1 05/12] bufmgr: Add Pin/UnpinLocalBuffer()
So far these were open-coded in quite a few places, without a good reason.
Author:
Reviewed-by:
Discussion: https://postgr.es/m/
Backpatch:
---
src/include/storage/buf_internals.h | 2 +
src/backend/storage/buffer/bufmgr.c | 30 +++----------
src/backend/storage/buffer/localbuf.c | 62 +++++++++++++++++----------
3 files changed, 46 insertions(+), 48 deletions(-)
diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.hindex 406db6be783..1cd6a968284 100644--- a/src/include/storage/buf_internals.h+++ b/src/include/storage/buf_internals.h@@ -413,6 +413,8 @@ extern int BufTableInsert(BufferTag *tagPtr, uint32 hashcode, int buf_id);
extern void BufTableDelete(BufferTag *tagPtr, uint32 hashcode);
/* localbuf.c */
+extern bool PinLocalBuffer(BufferDesc *buf_hdr, bool adjust_usagecount);+extern void UnpinLocalBuffer(Buffer buffer);
extern PrefetchBufferResult PrefetchLocalBuffer(SMgrRelation smgr,
ForkNumber forkNum,
BlockNumber blockNum);
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.cindex dfb7cfe0ef3..a7342388a41 100644--- a/src/backend/storage/buffer/bufmgr.c+++ b/src/backend/storage/buffer/bufmgr.c@@ -644,20 +644,7 @@ ReadRecentBuffer(RelFileLocator rlocator, ForkNumber forkNum, BlockNumber blockN
/* Is it still valid and holding the right tag? */
if ((buf_state & BM_VALID) && BufferTagsEqual(&tag, &bufHdr->tag))
{
- /*- * Bump buffer's ref and usage counts. This is equivalent of- * PinBuffer for a shared buffer.- */- if (LocalRefCount[b] == 0)- {- if (BUF_STATE_GET_USAGECOUNT(buf_state) < BM_MAX_USAGE_COUNT)- {- buf_state += BUF_USAGECOUNT_ONE;- pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state);- }- }- LocalRefCount[b]++;- ResourceOwnerRememberBuffer(CurrentResourceOwner, recent_buffer);+ PinLocalBuffer(bufHdr, true);
pgBufferUsage.local_blks_hit++;
@@ -1660,8 +1647,7 @@ ReleaseAndReadBuffer(Buffer buffer,
BufTagMatchesRelFileLocator(&bufHdr->tag, &relation->rd_locator) &&
BufTagGetForkNum(&bufHdr->tag) == forkNum)
return buffer;
- ResourceOwnerForgetBuffer(CurrentResourceOwner, buffer);- LocalRefCount[-buffer - 1]--;+ UnpinLocalBuffer(buffer);
}
else
{
@@ -3938,15 +3924,9 @@ ReleaseBuffer(Buffer buffer)
elog(ERROR, "bad buffer ID: %d", buffer);
if (BufferIsLocal(buffer))
- {- ResourceOwnerForgetBuffer(CurrentResourceOwner, buffer);-- Assert(LocalRefCount[-buffer - 1] > 0);- LocalRefCount[-buffer - 1]--;- return;- }-- UnpinBuffer(GetBufferDescriptor(buffer - 1));+ UnpinLocalBuffer(buffer);+ else+ UnpinBuffer(GetBufferDescriptor(buffer - 1));
}
/*
diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.cindex 30d67d1c40d..e6349536a16 100644--- a/src/backend/storage/buffer/localbuf.c+++ b/src/backend/storage/buffer/localbuf.c@@ -136,27 +136,8 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum,
fprintf(stderr, "LB ALLOC (%u,%d,%d) %d\n",
smgr->smgr_rlocator.locator.relNumber, forkNum, blockNum, -b - 1);
#endif
- buf_state = pg_atomic_read_u32(&bufHdr->state);- /* this part is equivalent to PinBuffer for a shared buffer */- if (LocalRefCount[b] == 0)- {- if (BUF_STATE_GET_USAGECOUNT(buf_state) < BM_MAX_USAGE_COUNT)- {- buf_state += BUF_USAGECOUNT_ONE;- pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state);- }- }- LocalRefCount[b]++;- ResourceOwnerRememberBuffer(CurrentResourceOwner,- BufferDescriptorGetBuffer(bufHdr));- if (buf_state & BM_VALID)- *foundPtr = true;- else- {- /* Previous read attempt must have failed; try again */- *foundPtr = false;- }+ *foundPtr = PinLocalBuffer(bufHdr, true);
return bufHdr;
}
@@ -193,9 +174,7 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum,
else
{
/* Found a usable buffer */
- LocalRefCount[b]++;- ResourceOwnerRememberBuffer(CurrentResourceOwner,- BufferDescriptorGetBuffer(bufHdr));+ PinLocalBuffer(bufHdr, false);
break;
}
}
@@ -483,6 +462,43 @@ InitLocalBuffers(void)
NLocBuffer = nbufs;
}
+bool+PinLocalBuffer(BufferDesc *buf_hdr, bool adjust_usagecount)+{+ uint32 buf_state;+ Buffer buffer = BufferDescriptorGetBuffer(buf_hdr);+ int bufid = -(buffer + 1);++ buf_state = pg_atomic_read_u32(&buf_hdr->state);++ if (LocalRefCount[bufid] == 0)+ {+ if (adjust_usagecount &&+ BUF_STATE_GET_USAGECOUNT(buf_state) < BM_MAX_USAGE_COUNT)+ {+ buf_state += BUF_USAGECOUNT_ONE;+ pg_atomic_unlocked_write_u32(&buf_hdr->state, buf_state);+ }+ }+ LocalRefCount[bufid]++;+ ResourceOwnerRememberBuffer(CurrentResourceOwner,+ BufferDescriptorGetBuffer(buf_hdr));++ return buf_state & BM_VALID;+}++void+UnpinLocalBuffer(Buffer buffer)+{+ int buffid = -buffer - 1;++ Assert(BufferIsLocal(buffer));+ Assert(LocalRefCount[buffid] > 0);++ ResourceOwnerForgetBuffer(CurrentResourceOwner, buffer);+ LocalRefCount[buffid]--;+}+
/*
* GUC check_hook for temp_buffers
*/
--
2.38.0
[text/x-diff] v1-0006-bufmgr-Acquire-and-clean-victim-buffer-separately.patch (27.1K, ../20221029025420.eplyow6k7tgu6he3@awork3.anarazel.de/7-v1-0006-bufmgr-Acquire-and-clean-victim-buffer-separately.patch)
download | inline diff:
From 7e4a1c30a70a1c60fcf6cd505bf0c17463e7342b Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Sun, 23 Oct 2022 14:29:57 -0700
Subject: [PATCH v1 06/12] bufmgr: Acquire and clean victim buffer separately
Previously we held buffer locks for two buffer mapping partitions at the same
time to change the identity of buffers. Particularly for extending relations
needing to hold the extension lock while acquiring a victim buffer is
painful. By separating out the victim buffer acquisition, future commits will
be able to change relation extensions to scale better.
Author:
Reviewed-by:
Discussion: https://postgr.es/m/
Backpatch:
---
src/backend/storage/buffer/bufmgr.c | 570 ++++++++++++++------------
src/backend/storage/buffer/localbuf.c | 115 +++---
2 files changed, 381 insertions(+), 304 deletions(-)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.cindex a7342388a41..50550a8a70e 100644--- a/src/backend/storage/buffer/bufmgr.c+++ b/src/backend/storage/buffer/bufmgr.c@@ -482,6 +482,7 @@ static BufferDesc *BufferAlloc(SMgrRelation smgr,
BlockNumber blockNum,
BufferAccessStrategy strategy,
bool *foundPtr);
+static Buffer GetVictimBuffer(BufferAccessStrategy strategy);
static void FlushBuffer(BufferDesc *buf, SMgrRelation reln);
static void FindAndDropRelationBuffers(RelFileLocator rlocator,
ForkNumber forkNum,
@@ -1111,14 +1112,11 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
BufferTag newTag; /* identity of requested block */
uint32 newHash; /* hash value for newTag */
LWLock *newPartitionLock; /* buffer partition lock for it */
- BufferTag oldTag; /* previous identity of selected buffer */- uint32 oldHash; /* hash value for oldTag */- LWLock *oldPartitionLock; /* buffer partition lock for it */- uint32 oldFlags;- int buf_id;- BufferDesc *buf;- bool valid;- uint32 buf_state;+ int existing_buf_id;++ Buffer victim_buffer;+ BufferDesc *victim_buf_hdr;+ uint32 victim_buf_state;
/* create a tag so we can lookup the buffer */
InitBufferTag(&newTag, &smgr->smgr_rlocator.locator, forkNum, blockNum);
@@ -1129,15 +1127,18 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
/* see if the block is in the buffer pool already */
LWLockAcquire(newPartitionLock, LW_SHARED);
- buf_id = BufTableLookup(&newTag, newHash);- if (buf_id >= 0)+ existing_buf_id = BufTableLookup(&newTag, newHash);+ if (existing_buf_id >= 0)
{
+ BufferDesc *buf;+ bool valid;+
/*
* Found it. Now, pin the buffer so no one can steal it from the
* buffer pool, and check to see if the correct data has been loaded
* into the buffer.
*/
- buf = GetBufferDescriptor(buf_id);+ buf = GetBufferDescriptor(existing_buf_id);
valid = PinBuffer(buf, strategy);
@@ -1174,266 +1175,96 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
*/
LWLockRelease(newPartitionLock);
- /* Loop here in case we have to try another victim buffer */- for (;;)+ /*+ * Acquire a victim buffer. Somebody else might try to do the same, we+ * don't hold any conflicting locks. If so we'll have to undo our work+ * later.+ */+ victim_buffer = GetVictimBuffer(strategy);+ victim_buf_hdr = GetBufferDescriptor(victim_buffer - 1);++ /*+ * Try to make a hashtable entry for the buffer under its new tag. If+ * somebody else inserted another buffer for the tag, we'll release the+ * victim buffer we acquired and use the already inserted one.+ */+ LWLockAcquire(newPartitionLock, LW_EXCLUSIVE);+ existing_buf_id = BufTableInsert(&newTag, newHash, victim_buf_hdr->buf_id);+ if (existing_buf_id >= 0)
{
- /*- * Ensure, while the spinlock's not yet held, that there's a free- * refcount entry.- */- ReservePrivateRefCountEntry();+ BufferDesc *existing_buf_hdr;+ bool valid;
/*
- * Select a victim buffer. The buffer is returned with its header- * spinlock still held!+ * Got a collision. Someone has already done what we were about to+ * do. We'll just handle this as if it were found in the buffer pool+ * in the first place. First, give up the buffer we were planning to+ * use.+ *+ * We could do this after releasing the partition lock, but then we'd+ * have to call ResourceOwnerEnlargeBuffers() &+ * ReservePrivateRefCountEntry() before acquiring the lock, for the+ * rare case of such a collision.
*/
- buf = StrategyGetBuffer(strategy, &buf_state);+ UnpinBuffer(victim_buf_hdr);- Assert(BUF_STATE_GET_REFCOUNT(buf_state) == 0);+ /* FIXME: Should we put the victim buffer onto the freelist? */- /* Must copy buffer flags while we still hold the spinlock */- oldFlags = buf_state & BUF_FLAG_MASK;+ /* remaining code should match code at top of routine */- /* Pin the buffer and then release the buffer spinlock */- PinBuffer_Locked(buf);+ existing_buf_hdr = GetBufferDescriptor(existing_buf_id);- /*- * If the buffer was dirty, try to write it out. There is a race- * condition here, in that someone might dirty it after we released it- * above, or even while we are writing it out (since our share-lock- * won't prevent hint-bit updates). We will recheck the dirty bit- * after re-locking the buffer header.- */- if (oldFlags & BM_DIRTY)- {- /*- * We need a share-lock on the buffer contents to write it out- * (else we might write invalid data, eg because someone else is- * compacting the page contents while we write). We must use a- * conditional lock acquisition here to avoid deadlock. Even- * though the buffer was not pinned (and therefore surely not- * locked) when StrategyGetBuffer returned it, someone else could- * have pinned and exclusive-locked it by the time we get here. If- * we try to get the lock unconditionally, we'd block waiting for- * them; if they later block waiting for us, deadlock ensues.- * (This has been observed to happen when two backends are both- * trying to split btree index pages, and the second one just- * happens to be trying to split the page the first one got from- * StrategyGetBuffer.)- */- if (LWLockConditionalAcquire(BufferDescriptorGetContentLock(buf),- LW_SHARED))- {- /*- * If using a nondefault strategy, and writing the buffer- * would require a WAL flush, let the strategy decide whether- * to go ahead and write/reuse the buffer or to choose another- * victim. We need lock to inspect the page LSN, so this- * can't be done inside StrategyGetBuffer.- */- if (strategy != NULL)- {- XLogRecPtr lsn;+ valid = PinBuffer(existing_buf_hdr, strategy);- /* Read the LSN while holding buffer header lock */- buf_state = LockBufHdr(buf);- lsn = BufferGetLSN(buf);- UnlockBufHdr(buf, buf_state);-- if (XLogNeedsFlush(lsn) &&- StrategyRejectBuffer(strategy, buf))- {- /* Drop lock/pin and loop around for another buffer */- LWLockRelease(BufferDescriptorGetContentLock(buf));- UnpinBuffer(buf);- continue;- }- }-- /* OK, do the I/O */- TRACE_POSTGRESQL_BUFFER_WRITE_DIRTY_START(forkNum, blockNum,- smgr->smgr_rlocator.locator.spcOid,- smgr->smgr_rlocator.locator.dbOid,- smgr->smgr_rlocator.locator.relNumber);-- FlushBuffer(buf, NULL);- LWLockRelease(BufferDescriptorGetContentLock(buf));-- ScheduleBufferTagForWriteback(&BackendWritebackContext,- &buf->tag);-- TRACE_POSTGRESQL_BUFFER_WRITE_DIRTY_DONE(forkNum, blockNum,- smgr->smgr_rlocator.locator.spcOid,- smgr->smgr_rlocator.locator.dbOid,- smgr->smgr_rlocator.locator.relNumber);- }- else- {- /*- * Someone else has locked the buffer, so give it up and loop- * back to get another one.- */- UnpinBuffer(buf);- continue;- }- }-- /*- * To change the association of a valid buffer, we'll need to have- * exclusive lock on both the old and new mapping partitions.- */- if (oldFlags & BM_TAG_VALID)- {- /*- * Need to compute the old tag's hashcode and partition lock ID.- * XXX is it worth storing the hashcode in BufferDesc so we need- * not recompute it here? Probably not.- */- oldTag = buf->tag;- oldHash = BufTableHashCode(&oldTag);- oldPartitionLock = BufMappingPartitionLock(oldHash);-- /*- * Must lock the lower-numbered partition first to avoid- * deadlocks.- */- if (oldPartitionLock < newPartitionLock)- {- LWLockAcquire(oldPartitionLock, LW_EXCLUSIVE);- LWLockAcquire(newPartitionLock, LW_EXCLUSIVE);- }- else if (oldPartitionLock > newPartitionLock)- {- LWLockAcquire(newPartitionLock, LW_EXCLUSIVE);- LWLockAcquire(oldPartitionLock, LW_EXCLUSIVE);- }- else- {- /* only one partition, only one lock */- LWLockAcquire(newPartitionLock, LW_EXCLUSIVE);- }- }- else- {- /* if it wasn't valid, we need only the new partition */- LWLockAcquire(newPartitionLock, LW_EXCLUSIVE);- /* remember we have no old-partition lock or tag */- oldPartitionLock = NULL;- /* keep the compiler quiet about uninitialized variables */- oldHash = 0;- }-- /*- * Try to make a hashtable entry for the buffer under its new tag.- * This could fail because while we were writing someone else- * allocated another buffer for the same block we want to read in.- * Note that we have not yet removed the hashtable entry for the old- * tag.- */- buf_id = BufTableInsert(&newTag, newHash, buf->buf_id);-- if (buf_id >= 0)- {- /*- * Got a collision. Someone has already done what we were about to- * do. We'll just handle this as if it were found in the buffer- * pool in the first place. First, give up the buffer we were- * planning to use.- */- UnpinBuffer(buf);-- /* Can give up that buffer's mapping partition lock now */- if (oldPartitionLock != NULL &&- oldPartitionLock != newPartitionLock)- LWLockRelease(oldPartitionLock);-- /* remaining code should match code at top of routine */-- buf = GetBufferDescriptor(buf_id);-- valid = PinBuffer(buf, strategy);-- /* Can release the mapping lock as soon as we've pinned it */- LWLockRelease(newPartitionLock);-- *foundPtr = true;-- if (!valid)- {- /*- * We can only get here if (a) someone else is still reading- * in the page, or (b) a previous read attempt failed. We- * have to wait for any active read attempt to finish, and- * then set up our own read attempt if the page is still not- * BM_VALID. StartBufferIO does it all.- */- if (StartBufferIO(buf, true))- {- /*- * If we get here, previous attempts to read the buffer- * must have failed ... but we shall bravely try again.- */- *foundPtr = false;- }- }-- return buf;- }-- /*- * Need to lock the buffer header too in order to change its tag.- */- buf_state = LockBufHdr(buf);-- /*- * Somebody could have pinned or re-dirtied the buffer while we were- * doing the I/O and making the new hashtable entry. If so, we can't- * recycle this buffer; we must undo everything we've done and start- * over with a new victim buffer.- */- oldFlags = buf_state & BUF_FLAG_MASK;- if (BUF_STATE_GET_REFCOUNT(buf_state) == 1 && !(oldFlags & BM_DIRTY))- break;-- UnlockBufHdr(buf, buf_state);- BufTableDelete(&newTag, newHash);- if (oldPartitionLock != NULL &&- oldPartitionLock != newPartitionLock)- LWLockRelease(oldPartitionLock);+ /* Can release the mapping lock as soon as we've pinned it */
LWLockRelease(newPartitionLock);
- UnpinBuffer(buf);++ *foundPtr = true;++ if (!valid)+ {+ /*+ * We can only get here if (a) someone else is still reading+ * in the page, or (b) a previous read attempt failed. We+ * have to wait for any active read attempt to finish, and+ * then set up our own read attempt if the page is still not+ * BM_VALID. StartBufferIO does it all.+ */+ if (StartBufferIO(existing_buf_hdr, true))+ {+ /*+ * If we get here, previous attempts to read the buffer+ * must have failed ... but we shall bravely try again.+ */+ *foundPtr = false;+ }+ }++ return existing_buf_hdr;
}
/*
- * Okay, it's finally safe to rename the buffer.- *- * Clearing BM_VALID here is necessary, clearing the dirtybits is just- * paranoia. We also reset the usage_count since any recency of use of- * the old content is no longer relevant. (The usage_count starts out at- * 1 so that the buffer can survive one clock-sweep pass.)- *+ * Need to lock the buffer header too in order to change its tag.+ */+ victim_buf_state = LockBufHdr(victim_buf_hdr);++ /* some sanity checks while we hold the buffer header lock */+ Assert(BUF_STATE_GET_REFCOUNT(victim_buf_state) == 1);+ Assert(!(victim_buf_state & (BM_TAG_VALID | BM_VALID | BM_DIRTY | BM_IO_IN_PROGRESS)));++ victim_buf_hdr->tag = newTag;++ /*
* Make sure BM_PERMANENT is set for buffers that must be written at every
* checkpoint. Unlogged buffers only need to be written at shutdown
* checkpoints, except for their "init" forks, which need to be treated
* just like permanent relations.
*/
- buf->tag = newTag;- buf_state &= ~(BM_VALID | BM_DIRTY | BM_JUST_DIRTIED |- BM_CHECKPOINT_NEEDED | BM_IO_ERROR | BM_PERMANENT |- BUF_USAGECOUNT_MASK);+ victim_buf_state |= BM_TAG_VALID | BUF_USAGECOUNT_ONE;
if (relpersistence == RELPERSISTENCE_PERMANENT || forkNum == INIT_FORKNUM)
- buf_state |= BM_TAG_VALID | BM_PERMANENT | BUF_USAGECOUNT_ONE;- else- buf_state |= BM_TAG_VALID | BUF_USAGECOUNT_ONE;+ victim_buf_state |= BM_PERMANENT;- UnlockBufHdr(buf, buf_state);-- if (oldPartitionLock != NULL)- {- BufTableDelete(&oldTag, oldHash);- if (oldPartitionLock != newPartitionLock)- LWLockRelease(oldPartitionLock);- }+ UnlockBufHdr(victim_buf_hdr, victim_buf_state);
LWLockRelease(newPartitionLock);
@@ -1443,12 +1274,12 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
* to read it before we did, so there's nothing left for BufferAlloc() to
* do.
*/
- if (StartBufferIO(buf, true))+ if (StartBufferIO(victim_buf_hdr, true))
*foundPtr = false;
else
*foundPtr = true;
- return buf;+ return victim_buf_hdr;
}
/*
@@ -1557,6 +1388,239 @@ retry:
StrategyFreeBuffer(buf);
}
+/*+ * Helper routine for GetVictimBuffer()+ *+ * Needs to be called with the buffer pinned, but without the buffer header+ * spinlock held.+ *+ * Returns true if the buffer can be reused, in which case the buffer is only+ * pinned by this backend and marked as invalid, false otherwise.+ */+static bool+InvalidateVictimBuffer(BufferDesc *buf_hdr)+{+ uint32 buf_state = pg_atomic_read_u32(&buf_hdr->state);++ Assert(BUF_STATE_GET_REFCOUNT(buf_state) > 0);+ Assert(GetPrivateRefCount(BufferDescriptorGetBuffer(buf_hdr)) == 1);++ /* can't change while we're holding the pin */+ if (buf_state & BM_TAG_VALID)+ {+ uint32 hash;+ LWLock *partition_lock;+ BufferTag tag;++ /* have buffer pinned, so it's safe to read tag without lock */+ tag = buf_hdr->tag;++ hash = BufTableHashCode(&tag);+ partition_lock = BufMappingPartitionLock(hash);++ LWLockAcquire(partition_lock, LW_EXCLUSIVE);++ /* lock the buffer header */+ buf_state = LockBufHdr(buf_hdr);++ Assert(BufferTagsEqual(&buf_hdr->tag, &tag));++ /*+ * We have the buffer pinned nobody else should have been able to+ * unset this concurrently.+ */+ Assert(buf_state & BM_TAG_VALID);+ Assert(BUF_STATE_GET_REFCOUNT(buf_state) > 0);++ /*+ * If somebody else pinned the buffer since, or even worse, dirtied it,+ * give up on this buffer: It's clearly in use.+ */+ if (BUF_STATE_GET_REFCOUNT(buf_state) != 1 || (buf_state & BM_DIRTY))+ {+ Assert(BUF_STATE_GET_REFCOUNT(buf_state) > 0);++ UnlockBufHdr(buf_hdr, buf_state);+ LWLockRelease(partition_lock);++ return false;+ }++ /*+ * 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);++ Assert(BUF_STATE_GET_REFCOUNT(buf_state) > 0);++ BufTableDelete(&tag, hash);++ LWLockRelease(partition_lock);+ }++ Assert(!(buf_state & (BM_DIRTY | BM_VALID | BM_TAG_VALID)));+ Assert(BUF_STATE_GET_REFCOUNT(buf_state) > 0);+ Assert(BUF_STATE_GET_REFCOUNT(pg_atomic_read_u32(&buf_hdr->state)) > 0);++ return true;+}++static Buffer+GetVictimBuffer(BufferAccessStrategy strategy)+{+ Buffer cur_buf;+ BufferDesc *cur_buf_hdr = NULL;+ uint32 cur_buf_state;++ /*+ * Ensure, while the spinlock's not yet held, that there's a free+ * refcount entry.+ */+ ReservePrivateRefCountEntry();+ ResourceOwnerEnlargeBuffers(CurrentResourceOwner);++again:++ /*+ * Select a victim buffer. The buffer is returned with its header+ * spinlock still held!+ */+ cur_buf_hdr = StrategyGetBuffer(strategy, &cur_buf_state);+ cur_buf = BufferDescriptorGetBuffer(cur_buf_hdr);++ Assert(BUF_STATE_GET_REFCOUNT(cur_buf_state) == 0);++ /* Pin the buffer and then release the buffer spinlock */+ PinBuffer_Locked(cur_buf_hdr);++ /*+ * We shouldn't have any other pins for this buffer.+ */+ BufferCheckOneLocalPin(cur_buf);++ /*+ * If the buffer was dirty, try to write it out. There is a race+ * condition here, in that someone might dirty it after we released the+ * buffer header lock above, or even while we are writing it out (since+ * our share-lock won't prevent hint-bit updates). We will recheck the+ * dirty bit after re-locking the buffer header.+ */+ if (cur_buf_state & BM_DIRTY)+ {+ LWLock *content_lock;++ Assert(cur_buf_state & BM_TAG_VALID);+ Assert(cur_buf_state & BM_VALID);++ /*+ * We need a share-lock on the buffer contents to write it out+ * (else we might write invalid data, eg because someone else is+ * compacting the page contents while we write). We must use a+ * conditional lock acquisition here to avoid deadlock. Even+ * though the buffer was not pinned (and therefore surely not+ * locked) when StrategyGetBuffer returned it, someone else could+ * have pinned and exclusive-locked it by the time we get here. If+ * we try to get the lock unconditionally, we'd block waiting for+ * them; if they later block waiting for us, deadlock ensues.+ * (This has been observed to happen when two backends are both+ * trying to split btree index pages, and the second one just+ * happens to be trying to split the page the first one got from+ * StrategyGetBuffer.)+ */+ content_lock = BufferDescriptorGetContentLock(cur_buf_hdr);+ if (!LWLockConditionalAcquire(content_lock, LW_SHARED))+ {+ /*+ * Someone else has locked the buffer, so give it up and loop+ * back to get another one.+ */+ UnpinBuffer(cur_buf_hdr);+ goto again;+ }++ /*+ * If using a nondefault strategy, and writing the buffer would+ * require a WAL flush, let the strategy decide whether to go ahead+ * and write/reuse the buffer or to choose another victim. We need+ * lock to inspect the page LSN, so this can't be done inside+ * StrategyGetBuffer.+ */+ if (strategy != NULL)+ {+ XLogRecPtr lsn;++ /* Read the LSN while holding buffer header lock */+ cur_buf_state = LockBufHdr(cur_buf_hdr);+ lsn = BufferGetLSN(cur_buf_hdr);+ UnlockBufHdr(cur_buf_hdr, cur_buf_state);++ if (XLogNeedsFlush(lsn)+ && StrategyRejectBuffer(strategy, cur_buf_hdr))+ {+ LWLockRelease(content_lock);+ UnpinBuffer(cur_buf_hdr);+ goto again;+ }+ }++ /* OK, do the I/O */+ /* FIXME: These used the wrong smgr before afaict? */+ {+ SMgrRelation smgr = smgropen(BufTagGetRelFileLocator(&cur_buf_hdr->tag),+ InvalidBackendId);++ TRACE_POSTGRESQL_BUFFER_WRITE_DIRTY_START(cur_buf_hdr->tag.forkNum,+ cur_buf_hdr->tag.blockNum,+ smgr->smgr_rlocator.locator.spcOid,+ smgr->smgr_rlocator.locator.dbOid,+ smgr->smgr_rlocator.locator.relNumber);++ FlushBuffer(cur_buf_hdr, smgr);+ LWLockRelease(content_lock);++ ScheduleBufferTagForWriteback(&BackendWritebackContext,+ &cur_buf_hdr->tag);++ TRACE_POSTGRESQL_BUFFER_WRITE_DIRTY_DONE(cur_buf_hdr->tag.forkNum,+ cur_buf_hdr->tag.blockNum,+ smgr->smgr_rlocator.locator.spcOid,+ smgr->smgr_rlocator.locator.dbOid,+ smgr->smgr_rlocator.locator.relNumber);+ }+ }++ /*+ * If the buffer has an entry in the buffer mapping table, delete it. This+ * can fail because another backend could have pinned or dirtied the+ * buffer.+ */+ if (!InvalidateVictimBuffer(cur_buf_hdr))+ {+ UnpinBuffer(cur_buf_hdr);+ goto again;+ }++ /* a final set of sanity checks */+ cur_buf_state = pg_atomic_read_u32(&cur_buf_hdr->state);++ Assert(BUF_STATE_GET_REFCOUNT(cur_buf_state) == 1);+ Assert(!(cur_buf_state & (BM_TAG_VALID | BM_VALID | BM_DIRTY)));++ BufferCheckOneLocalPin(cur_buf);++ return cur_buf;+}
/*
* MarkBufferDirty
*
diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.cindex e6349536a16..6a3f325ea6d 100644--- a/src/backend/storage/buffer/localbuf.c+++ b/src/backend/storage/buffer/localbuf.c@@ -44,13 +44,14 @@ BufferDesc *LocalBufferDescriptors = NULL;
Block *LocalBufferBlockPointers = NULL;
int32 *LocalRefCount = NULL;
-static int nextFreeLocalBuf = 0;+static int nextFreeLocalBufId = 0;
static HTAB *LocalBufHash = NULL;
static void InitLocalBuffers(void);
static Block GetLocalBufferStorage(void);
+static Buffer GetLocalVictimBuffer(void);
/*
@@ -112,10 +113,9 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum,
BufferTag newTag; /* identity of requested block */
LocalBufferLookupEnt *hresult;
BufferDesc *bufHdr;
- int b;- int trycounter;+ Buffer victim_buffer;+ int bufid;
bool found;
- uint32 buf_state;
InitBufferTag(&newTag, &smgr->smgr_rlocator.locator, forkNum, blockNum);
@@ -129,23 +129,51 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum,
if (hresult)
{
- b = hresult->id;- bufHdr = GetLocalBufferDescriptor(b);+ bufid = hresult->id;+ bufHdr = GetLocalBufferDescriptor(bufid);
Assert(BufferTagsEqual(&bufHdr->tag, &newTag));
-#ifdef LBDEBUG- fprintf(stderr, "LB ALLOC (%u,%d,%d) %d\n",- smgr->smgr_rlocator.locator.relNumber, forkNum, blockNum, -b - 1);-#endif
*foundPtr = PinLocalBuffer(bufHdr, true);
- return bufHdr;+ }+ else+ {+ uint32 buf_state;++ victim_buffer = GetLocalVictimBuffer();+ bufid = -(victim_buffer + 1);+ bufHdr = GetLocalBufferDescriptor(bufid);++ hresult = (LocalBufferLookupEnt *)+ hash_search(LocalBufHash, (void *) &newTag, HASH_ENTER, &found);+ if (found) /* shouldn't happen */+ elog(ERROR, "local buffer hash table corrupted");+ hresult->id = bufid;++ /*+ * it's all ours now.+ */+ bufHdr->tag = newTag;++ buf_state = pg_atomic_read_u32(&bufHdr->state);+ buf_state &= ~(BUF_FLAG_MASK | BUF_USAGECOUNT_MASK);+ buf_state |= BM_TAG_VALID | BUF_USAGECOUNT_ONE;+ pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state);++ *foundPtr = false;
}
-#ifdef LBDEBUG- fprintf(stderr, "LB ALLOC (%u,%d,%d) %d\n",- smgr->smgr_rlocator.locator.relNumber, forkNum, blockNum,- -nextFreeLocalBuf - 1);-#endif+ return bufHdr;+}++static Buffer+GetLocalVictimBuffer(void)+{+ int victim_bufid;+ int trycounter;+ uint32 buf_state;+ BufferDesc *bufHdr;++ ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
/*
* Need to get a new buffer. We use a clock sweep algorithm (essentially
@@ -154,14 +182,14 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum,
trycounter = NLocBuffer;
for (;;)
{
- b = nextFreeLocalBuf;+ victim_bufid = nextFreeLocalBufId;- if (++nextFreeLocalBuf >= NLocBuffer)- nextFreeLocalBuf = 0;+ if (++nextFreeLocalBufId >= NLocBuffer)+ nextFreeLocalBufId = 0;- bufHdr = GetLocalBufferDescriptor(b);+ bufHdr = GetLocalBufferDescriptor(victim_bufid);- if (LocalRefCount[b] == 0)+ if (LocalRefCount[victim_bufid] == 0)
{
buf_state = pg_atomic_read_u32(&bufHdr->state);
@@ -184,6 +212,15 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum,
errmsg("no empty local buffer available")));
}
+ /*+ * lazy memory allocation: allocate space on first use of a buffer.+ */+ if (LocalBufHdrGetBlock(bufHdr) == NULL)+ {+ /* Set pointer for use by BufferGetBlock() macro */+ LocalBufHdrGetBlock(bufHdr) = GetLocalBufferStorage();+ }+
/*
* this buffer is not referenced but it might still be dirty. if that's
* the case, write it out before reusing it!
@@ -213,19 +250,12 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum,
}
/*
- * lazy memory allocation: allocate space on first use of a buffer.- */- if (LocalBufHdrGetBlock(bufHdr) == NULL)- {- /* Set pointer for use by BufferGetBlock() macro */- LocalBufHdrGetBlock(bufHdr) = GetLocalBufferStorage();- }-- /*- * Update the hash table: remove old entry, if any, and make new one.+ * Remove the victim buffer from the hashtable and mark as invalid.
*/
if (buf_state & BM_TAG_VALID)
{
+ LocalBufferLookupEnt *hresult;+
hresult = (LocalBufferLookupEnt *)
hash_search(LocalBufHash, (void *) &bufHdr->tag,
HASH_REMOVE, NULL);
@@ -233,28 +263,11 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum,
elog(ERROR, "local buffer hash table corrupted");
/* mark buffer invalid just in case hash insert fails */
ClearBufferTag(&bufHdr->tag);
- buf_state &= ~(BM_VALID | BM_TAG_VALID);+ buf_state &= ~(BUF_FLAG_MASK | BUF_USAGECOUNT_MASK);
pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state);
}
- hresult = (LocalBufferLookupEnt *)- hash_search(LocalBufHash, (void *) &newTag, HASH_ENTER, &found);- if (found) /* shouldn't happen */- elog(ERROR, "local buffer hash table corrupted");- hresult->id = b;-- /*- * it's all ours now.- */- bufHdr->tag = newTag;- buf_state &= ~(BM_VALID | BM_DIRTY | BM_JUST_DIRTIED | BM_IO_ERROR);- buf_state |= BM_TAG_VALID;- buf_state &= ~BUF_USAGECOUNT_MASK;- buf_state += BUF_USAGECOUNT_ONE;- pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state);-- *foundPtr = false;- return bufHdr;+ return BufferDescriptorGetBuffer(bufHdr);
}
/*
@@ -423,7 +436,7 @@ InitLocalBuffers(void)
(errcode(ERRCODE_OUT_OF_MEMORY),
errmsg("out of memory")));
- nextFreeLocalBuf = 0;+ nextFreeLocalBufId = 0;
/* initialize fields that need to start off nonzero */
for (i = 0; i < nbufs; i++)
--
2.38.0
[text/x-diff] v1-0007-bufmgr-Support-multiple-in-progress-IOs-by-using-.patch (11.7K, ../20221029025420.eplyow6k7tgu6he3@awork3.anarazel.de/8-v1-0007-bufmgr-Support-multiple-in-progress-IOs-by-using-.patch)
download | inline diff:
From 40b53b7cfdbe8ff8090e31e593f82477f6a574aa Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Mon, 24 Oct 2022 16:44:16 -0700
Subject: [PATCH v1 07/12] bufmgr: Support multiple in-progress IOs by using
resowner
---
src/include/storage/bufmgr.h | 2 +-
src/include/utils/resowner_private.h | 5 ++
src/backend/access/transam/xact.c | 4 +-
src/backend/postmaster/autovacuum.c | 1 -
src/backend/postmaster/bgwriter.c | 1 -
src/backend/postmaster/checkpointer.c | 1 -
src/backend/postmaster/walwriter.c | 1 -
src/backend/storage/buffer/bufmgr.c | 86 ++++++++++++---------------
src/backend/utils/resowner/resowner.c | 58 ++++++++++++++++++
9 files changed, 103 insertions(+), 56 deletions(-)
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.hindex 2c1f12d3066..723243aeb96 100644--- a/src/include/storage/bufmgr.h+++ b/src/include/storage/bufmgr.h@@ -167,7 +167,7 @@ extern bool ConditionalLockBufferForCleanup(Buffer buffer);
extern bool IsBufferCleanupOK(Buffer buffer);
extern bool HoldingBufferPinThatDelaysRecovery(void);
-extern void AbortBufferIO(void);+extern void AbortBufferIO(Buffer buffer);
extern void BufmgrCommit(void);
extern bool BgBufferSync(struct WritebackContext *wb_context);
diff --git a/src/include/utils/resowner_private.h b/src/include/utils/resowner_private.hindex d01cccc27c1..61a72ed52f5 100644--- a/src/include/utils/resowner_private.h+++ b/src/include/utils/resowner_private.h@@ -30,6 +30,11 @@ extern void ResourceOwnerEnlargeBuffers(ResourceOwner owner);
extern void ResourceOwnerRememberBuffer(ResourceOwner owner, Buffer buffer);
extern void ResourceOwnerForgetBuffer(ResourceOwner owner, Buffer buffer);
+/* support for IO-in-progress management */+extern void ResourceOwnerEnlargeBufferIOs(ResourceOwner owner);+extern void ResourceOwnerRememberBufferIO(ResourceOwner owner, Buffer buffer);+extern void ResourceOwnerForgetBufferIO(ResourceOwner owner, Buffer buffer);+
/* support for local lock management */
extern void ResourceOwnerRememberLock(ResourceOwner owner, LOCALLOCK *locallock);
extern void ResourceOwnerForgetLock(ResourceOwner owner, LOCALLOCK *locallock);
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.cindex fd5103a78e2..71b298d38fe 100644--- a/src/backend/access/transam/xact.c+++ b/src/backend/access/transam/xact.c@@ -2707,8 +2707,7 @@ AbortTransaction(void)
pgstat_report_wait_end();
pgstat_progress_end_command();
- /* Clean up buffer I/O and buffer context locks, too */- AbortBufferIO();+ /* Clean up buffer context locks, too */
UnlockBuffers();
/* Reset WAL record construction state */
@@ -5051,7 +5050,6 @@ AbortSubTransaction(void)
pgstat_report_wait_end();
pgstat_progress_end_command();
- AbortBufferIO();
UnlockBuffers();
/* Reset WAL record construction state */
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.cindex 1e90b72b740..09b8986597a 100644--- a/src/backend/postmaster/autovacuum.c+++ b/src/backend/postmaster/autovacuum.c@@ -526,7 +526,6 @@ AutoVacLauncherMain(int argc, char *argv[])
*/
LWLockReleaseAll();
pgstat_report_wait_end();
- AbortBufferIO();
UnlockBuffers();
/* this is probably dead code, but let's be safe: */
if (AuxProcessResourceOwner)
diff --git a/src/backend/postmaster/bgwriter.c b/src/backend/postmaster/bgwriter.cindex 91e6f6ea18a..372cf21464e 100644--- a/src/backend/postmaster/bgwriter.c+++ b/src/backend/postmaster/bgwriter.c@@ -167,7 +167,6 @@ BackgroundWriterMain(void)
*/
LWLockReleaseAll();
ConditionVariableCancelSleep();
- AbortBufferIO();
UnlockBuffers();
ReleaseAuxProcessResources(false);
AtEOXact_Buffers(false);
diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.cindex 5fc076fc149..a3954e7cc0e 100644--- a/src/backend/postmaster/checkpointer.c+++ b/src/backend/postmaster/checkpointer.c@@ -271,7 +271,6 @@ CheckpointerMain(void)
LWLockReleaseAll();
ConditionVariableCancelSleep();
pgstat_report_wait_end();
- AbortBufferIO();
UnlockBuffers();
ReleaseAuxProcessResources(false);
AtEOXact_Buffers(false);
diff --git a/src/backend/postmaster/walwriter.c b/src/backend/postmaster/walwriter.cindex beb46dcb55c..2bea4947c1b 100644--- a/src/backend/postmaster/walwriter.c+++ b/src/backend/postmaster/walwriter.c@@ -163,7 +163,6 @@ WalWriterMain(void)
LWLockReleaseAll();
ConditionVariableCancelSleep();
pgstat_report_wait_end();
- AbortBufferIO();
UnlockBuffers();
ReleaseAuxProcessResources(false);
AtEOXact_Buffers(false);
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.cindex 50550a8a70e..b1404bd77dd 100644--- a/src/backend/storage/buffer/bufmgr.c+++ b/src/backend/storage/buffer/bufmgr.c@@ -159,10 +159,6 @@ int checkpoint_flush_after = 0;
int bgwriter_flush_after = 0;
int backend_flush_after = 0;
-/* local state for StartBufferIO and related functions */-static BufferDesc *InProgressBuf = NULL;-static bool IsForInput;-
/* local state for LockBufferForCleanup */
static BufferDesc *PinCountWaitBuf = NULL;
@@ -2689,7 +2685,6 @@ InitBufferPoolAccess(void)
static void
AtProcExit_Buffers(int code, Datum arg)
{
- AbortBufferIO();
UnlockBuffers();
CheckForBufferLeaks();
@@ -4618,7 +4613,7 @@ StartBufferIO(BufferDesc *buf, bool forInput)
{
uint32 buf_state;
- Assert(!InProgressBuf);+ ResourceOwnerEnlargeBufferIOs(CurrentResourceOwner);
for (;;)
{
@@ -4642,8 +4637,8 @@ StartBufferIO(BufferDesc *buf, bool forInput)
buf_state |= BM_IO_IN_PROGRESS;
UnlockBufHdr(buf, buf_state);
- InProgressBuf = buf;- IsForInput = forInput;+ ResourceOwnerRememberBufferIO(CurrentResourceOwner,+ BufferDescriptorGetBuffer(buf));
return true;
}
@@ -4669,8 +4664,6 @@ TerminateBufferIO(BufferDesc *buf, bool clear_dirty, uint32 set_flag_bits)
{
uint32 buf_state;
- Assert(buf == InProgressBuf);-
buf_state = LockBufHdr(buf);
Assert(buf_state & BM_IO_IN_PROGRESS);
@@ -4682,13 +4675,14 @@ TerminateBufferIO(BufferDesc *buf, bool clear_dirty, uint32 set_flag_bits)
buf_state |= set_flag_bits;
UnlockBufHdr(buf, buf_state);
- InProgressBuf = NULL;+ ResourceOwnerForgetBufferIO(CurrentResourceOwner,+ BufferDescriptorGetBuffer(buf));
ConditionVariableBroadcast(BufferDescriptorGetIOCV(buf));
}
/*
- * AbortBufferIO: Clean up any active buffer I/O after an error.+ * AbortBufferIO: Clean up active buffer I/O after an error.
*
* All LWLocks we might have held have been released,
* but we haven't yet released buffer pins, so the buffer is still pinned.
@@ -4697,46 +4691,42 @@ TerminateBufferIO(BufferDesc *buf, bool clear_dirty, uint32 set_flag_bits)
* possible the error condition wasn't related to the I/O.
*/
void
-AbortBufferIO(void)+AbortBufferIO(Buffer buf)
{
- BufferDesc *buf = InProgressBuf;+ BufferDesc *buf_hdr = GetBufferDescriptor(buf - 1);+ uint32 buf_state;- if (buf)+ buf_state = LockBufHdr(buf_hdr);+ Assert(buf_state & (BM_IO_IN_PROGRESS | BM_TAG_VALID));++ if (!(buf_state & BM_VALID))
{
- uint32 buf_state;-- buf_state = LockBufHdr(buf);- Assert(buf_state & BM_IO_IN_PROGRESS);- if (IsForInput)- {- Assert(!(buf_state & BM_DIRTY));-- /* We'd better not think buffer is valid yet */- Assert(!(buf_state & BM_VALID));- UnlockBufHdr(buf, buf_state);- }- else- {- Assert(buf_state & BM_DIRTY);- UnlockBufHdr(buf, buf_state);- /* Issue notice if this is not the first failure... */- if (buf_state & BM_IO_ERROR)- {- /* Buffer is pinned, so we can read tag without spinlock */- char *path;-- path = relpathperm(BufTagGetRelFileLocator(&buf->tag),- BufTagGetForkNum(&buf->tag));- ereport(WARNING,- (errcode(ERRCODE_IO_ERROR),- errmsg("could not write block %u of %s",- buf->tag.blockNum, path),- errdetail("Multiple failures --- write error might be permanent.")));- pfree(path);- }- }- TerminateBufferIO(buf, false, BM_IO_ERROR);+ Assert(!(buf_state & BM_DIRTY));+ UnlockBufHdr(buf_hdr, buf_state);
}
+ else+ {+ Assert(!(buf_state & BM_DIRTY));+ UnlockBufHdr(buf_hdr, buf_state);++ /* Issue notice if this is not the first failure... */+ if (buf_state & BM_IO_ERROR)+ {+ /* Buffer is pinned, so we can read tag without spinlock */+ char *path;++ path = relpathperm(BufTagGetRelFileLocator(&buf_hdr->tag),+ BufTagGetForkNum(&buf_hdr->tag));+ ereport(WARNING,+ (errcode(ERRCODE_IO_ERROR),+ errmsg("could not write block %u of %s",+ buf_hdr->tag.blockNum, path),+ errdetail("Multiple failures --- write error might be permanent.")));+ pfree(path);+ }+ }++ TerminateBufferIO(buf_hdr, false, BM_IO_ERROR);
}
/*
diff --git a/src/backend/utils/resowner/resowner.c b/src/backend/utils/resowner/resowner.cindex 37b43ee1f8c..62311831348 100644--- a/src/backend/utils/resowner/resowner.c+++ b/src/backend/utils/resowner/resowner.c@@ -121,6 +121,7 @@ typedef struct ResourceOwnerData
/* We have built-in support for remembering: */
ResourceArray bufferarr; /* owned buffers */
+ ResourceArray bufferioarr; /* in-progress buffer IO */
ResourceArray catrefarr; /* catcache references */
ResourceArray catlistrefarr; /* catcache-list pins */
ResourceArray relrefarr; /* relcache references */
@@ -441,6 +442,7 @@ ResourceOwnerCreate(ResourceOwner parent, const char *name)
}
ResourceArrayInit(&(owner->bufferarr), BufferGetDatum(InvalidBuffer));
+ ResourceArrayInit(&(owner->bufferioarr), BufferGetDatum(InvalidBuffer));
ResourceArrayInit(&(owner->catrefarr), PointerGetDatum(NULL));
ResourceArrayInit(&(owner->catlistrefarr), PointerGetDatum(NULL));
ResourceArrayInit(&(owner->relrefarr), PointerGetDatum(NULL));
@@ -517,6 +519,24 @@ ResourceOwnerReleaseInternal(ResourceOwner owner,
if (phase == RESOURCE_RELEASE_BEFORE_LOCKS)
{
+ /*+ * Abort failed buffer IO. AbortBufferIO()->TerminateBufferIO() calls+ * ResourceOwnerForgetBufferIOs(), so we just have to iterate till+ * there are none.+ *+ * Needs to be before we release buffer pins.+ *+ * During a commit, there shouldn't be any in-progress IO.+ */+ while (ResourceArrayGetAny(&(owner->bufferioarr), &foundres))+ {+ Buffer res = DatumGetBuffer(foundres);++ if (isCommit)+ elog(PANIC, "lost track of buffer IO on buffer %u", res);+ AbortBufferIO(res);+ }+
/*
* Release buffer pins. Note that ReleaseBuffer will remove the
* buffer entry from our array, so we just have to iterate till there
@@ -976,6 +996,44 @@ ResourceOwnerForgetBuffer(ResourceOwner owner, Buffer buffer)
buffer, owner->name);
}
++/*+ * Make sure there is room for at least one more entry in a ResourceOwner's+ * buffer array.+ *+ * This is separate from actually inserting an entry because if we run out+ * of memory, it's critical to do so *before* acquiring the resource.+ */+void+ResourceOwnerEnlargeBufferIOs(ResourceOwner owner)+{+ /* We used to allow pinning buffers without a resowner, but no more */+ Assert(owner != NULL);+ ResourceArrayEnlarge(&(owner->bufferioarr));+}++/*+ * Remember that a buffer IO is owned by a ResourceOwner+ *+ * Caller must have previously done ResourceOwnerEnlargeBufferIOs()+ */+void+ResourceOwnerRememberBufferIO(ResourceOwner owner, Buffer buffer)+{+ ResourceArrayAdd(&(owner->bufferioarr), BufferGetDatum(buffer));+}++/*+ * Forget that a buffer IO is owned by a ResourceOwner+ */+void+ResourceOwnerForgetBufferIO(ResourceOwner owner, Buffer buffer)+{+ if (!ResourceArrayRemove(&(owner->bufferioarr), BufferGetDatum(buffer)))+ elog(PANIC, "buffer IO %d is not owned by resource owner %s",+ buffer, owner->name);+}+
/*
* Remember that a Local Lock is owned by a ResourceOwner
*
--
2.38.0
[text/x-diff] v1-0008-bufmgr-Move-relation-extension-handling-into-Bulk.patch (31.5K, ../20221029025420.eplyow6k7tgu6he3@awork3.anarazel.de/9-v1-0008-bufmgr-Move-relation-extension-handling-into-Bulk.patch)
download | inline diff:
From e5c1076a9961aba8175e723bd4cdda4bd438c288 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Wed, 26 Oct 2022 14:44:02 -0700
Subject: [PATCH v1 08/12] bufmgr: Move relation extension handling into
[Bulk]ExtendRelationBuffered()
---
src/include/storage/buf_internals.h | 5 +
src/include/storage/bufmgr.h | 13 +
src/backend/storage/buffer/bufmgr.c | 546 ++++++++++++++++++--------
src/backend/storage/buffer/localbuf.c | 134 ++++++-
src/backend/utils/probes.d | 6 +-
doc/src/sgml/monitoring.sgml | 11 +-
6 files changed, 546 insertions(+), 169 deletions(-)
diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.hindex 1cd6a968284..34b28eea906 100644--- a/src/include/storage/buf_internals.h+++ b/src/include/storage/buf_internals.h@@ -420,6 +420,11 @@ extern PrefetchBufferResult PrefetchLocalBuffer(SMgrRelation smgr,
BlockNumber blockNum);
extern BufferDesc *LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum,
BlockNumber blockNum, bool *foundPtr);
+extern BlockNumber BulkExtendLocalRelationBuffered(SMgrRelation smgr,+ ForkNumber fork,+ ReadBufferMode mode,+ uint32 *num_pages,+ Buffer *buffers);
extern void MarkLocalBufferDirty(Buffer buffer);
extern void DropRelationLocalBuffers(RelFileLocator rlocator,
ForkNumber forkNum,
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.hindex 723243aeb96..a9cb876c777 100644--- a/src/include/storage/bufmgr.h+++ b/src/include/storage/bufmgr.h@@ -123,6 +123,19 @@ extern void IncrBufferRefCount(Buffer buffer);
extern void BufferCheckOneLocalPin(Buffer buffer);
extern Buffer ReleaseAndReadBuffer(Buffer buffer, Relation relation,
BlockNumber blockNum);
+extern Buffer ExtendRelationBuffered(Relation reln, struct SMgrRelationData *smgr,+ bool skip_extension_lock,+ char relpersistence,+ ForkNumber forkNum, ReadBufferMode mode,+ BufferAccessStrategy strategy);+extern BlockNumber BulkExtendRelationBuffered(Relation rel, struct SMgrRelationData *smgr,+ bool skip_extension_lock,+ char relpersistence,+ ForkNumber fork, ReadBufferMode mode,+ BufferAccessStrategy strategy,+ uint32 *num_pages,+ uint32 num_locked_pages,+ Buffer *buffers);
extern void InitBufferPoolAccess(void);
extern void AtEOXact_Buffers(bool isCommit);
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.cindex b1404bd77dd..196b7d1ff25 100644--- a/src/backend/storage/buffer/bufmgr.c+++ b/src/backend/storage/buffer/bufmgr.c@@ -48,6 +48,7 @@
#include "storage/buf_internals.h"
#include "storage/bufmgr.h"
#include "storage/ipc.h"
+#include "storage/lmgr.h"
#include "storage/proc.h"
#include "storage/smgr.h"
#include "storage/standby.h"
@@ -459,6 +460,15 @@ static Buffer ReadBuffer_common(SMgrRelation smgr, char relpersistence,
ForkNumber forkNum, BlockNumber blockNum,
ReadBufferMode mode, BufferAccessStrategy strategy,
bool *hit);
+static BlockNumber BulkExtendSharedRelationBuffered(Relation rel,+ SMgrRelation smgr,+ bool skip_extension_lock,+ char relpersistence,+ ForkNumber fork, ReadBufferMode mode,+ BufferAccessStrategy strategy,+ uint32 *num_pages,+ uint32 num_locked_pages,+ Buffer *buffers);
static bool PinBuffer(BufferDesc *buf, BufferAccessStrategy strategy);
static void PinBuffer_Locked(BufferDesc *buf);
static void UnpinBuffer(BufferDesc *buf);
@@ -793,6 +803,73 @@ ReadBufferWithoutRelcache(RelFileLocator rlocator, ForkNumber forkNum,
mode, strategy, &hit);
}
+/*+ * Convenience wrapper around BulkExtendRelationBuffered() extending by one+ * block.+ */+Buffer+ExtendRelationBuffered(Relation rel, struct SMgrRelationData *smgr,+ bool skip_extension_lock,+ char relpersistence, ForkNumber forkNum,+ ReadBufferMode mode, BufferAccessStrategy strategy)+{+ Buffer buf;+ uint32 num_pages = 1;++ BulkExtendRelationBuffered(rel, smgr, skip_extension_lock, relpersistence,+ forkNum, mode, strategy, &num_pages, num_pages, &buf);++ return buf;+}+++BlockNumber+BulkExtendRelationBuffered(Relation rel,+ SMgrRelation smgr,+ bool skip_extension_lock,+ char relpersistence,+ ForkNumber fork, ReadBufferMode mode,+ BufferAccessStrategy strategy,+ uint32 *num_pages,+ uint32 num_locked_pages,+ Buffer *buffers)+{+ BlockNumber first_block;++ Assert(rel != NULL || smgr != NULL);+ Assert(rel != NULL || skip_extension_lock);++ if (smgr == NULL)+ smgr = RelationGetSmgr(rel);++ TRACE_POSTGRESQL_BUFFER_EXTEND_START(fork,+ smgr->smgr_rlocator.locator.spcOid,+ smgr->smgr_rlocator.locator.dbOid,+ smgr->smgr_rlocator.locator.relNumber,+ smgr->smgr_rlocator.backend,+ num_pages);++ if (SmgrIsTemp(smgr))+ first_block = BulkExtendLocalRelationBuffered(smgr,+ fork, mode,+ num_pages, buffers);+ else+ first_block = BulkExtendSharedRelationBuffered(rel, smgr,+ skip_extension_lock, relpersistence,+ fork, mode, strategy,+ num_pages, num_locked_pages,+ buffers);++ TRACE_POSTGRESQL_BUFFER_EXTEND_DONE(fork,+ smgr->smgr_rlocator.locator.spcOid,+ smgr->smgr_rlocator.locator.dbOid,+ smgr->smgr_rlocator.locator.relNumber,+ smgr->smgr_rlocator.backend,+ num_pages,+ first_block);++ return first_block;+}
/*
* ReadBuffer_common -- common logic for all ReadBuffer variants
@@ -807,43 +884,32 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
BufferDesc *bufHdr;
Block bufBlock;
bool found;
- bool isExtend;
bool isLocalBuf = SmgrIsTemp(smgr);
*hit = false;
+ /*+ * Backward compatibility path, most code should use+ * ExtendRelationBuffered() instead, as acquiring the extension lock+ * inside ExtendRelationBuffered() scales a lot better.+ */+ if (unlikely(blockNum == P_NEW))+ return ExtendRelationBuffered(NULL, smgr, true, relpersistence, forkNum, mode, strategy);+
/* Make sure we will have room to remember the buffer pin */
ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
- isExtend = (blockNum == P_NEW);-
TRACE_POSTGRESQL_BUFFER_READ_START(forkNum, blockNum,
smgr->smgr_rlocator.locator.spcOid,
smgr->smgr_rlocator.locator.dbOid,
smgr->smgr_rlocator.locator.relNumber,
- smgr->smgr_rlocator.backend,- isExtend);-- /* Substitute proper block number if caller asked for P_NEW */- if (isExtend)- {- blockNum = smgrnblocks(smgr, forkNum);- /* Fail if relation is already at maximum possible length */- if (blockNum == P_NEW)- ereport(ERROR,- (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),- errmsg("cannot extend relation %s beyond %u blocks",- relpath(smgr->smgr_rlocator, forkNum),- P_NEW)));- }+ smgr->smgr_rlocator.backend);
if (isLocalBuf)
{
bufHdr = LocalBufferAlloc(smgr, forkNum, blockNum, &found);
if (found)
pgBufferUsage.local_blks_hit++;
- else if (isExtend)- pgBufferUsage.local_blks_written++;
else if (mode == RBM_NORMAL || mode == RBM_NORMAL_NO_LOG ||
mode == RBM_ZERO_ON_ERROR)
pgBufferUsage.local_blks_read++;
@@ -858,8 +924,6 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
strategy, &found);
if (found)
pgBufferUsage.shared_blks_hit++;
- else if (isExtend)- pgBufferUsage.shared_blks_written++;
else if (mode == RBM_NORMAL || mode == RBM_NORMAL_NO_LOG ||
mode == RBM_ZERO_ON_ERROR)
pgBufferUsage.shared_blks_read++;
@@ -870,168 +934,88 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
/* if it was already in the buffer pool, we're done */
if (found)
{
- if (!isExtend)- {- /* Just need to update stats before we exit */- *hit = true;- VacuumPageHit++;+ /* Just need to update stats before we exit */+ *hit = true;+ VacuumPageHit++;- if (VacuumCostActive)- VacuumCostBalance += VacuumCostPageHit;+ if (VacuumCostActive)+ VacuumCostBalance += VacuumCostPageHit;- TRACE_POSTGRESQL_BUFFER_READ_DONE(forkNum, blockNum,- smgr->smgr_rlocator.locator.spcOid,- smgr->smgr_rlocator.locator.dbOid,- smgr->smgr_rlocator.locator.relNumber,- smgr->smgr_rlocator.backend,- isExtend,- found);-- /*- * In RBM_ZERO_AND_LOCK mode the caller expects the page to be- * locked on return.- */- if (!isLocalBuf)- {- if (mode == RBM_ZERO_AND_LOCK)- LWLockAcquire(BufferDescriptorGetContentLock(bufHdr),- LW_EXCLUSIVE);- else if (mode == RBM_ZERO_AND_CLEANUP_LOCK)- LockBufferForCleanup(BufferDescriptorGetBuffer(bufHdr));- }-- return BufferDescriptorGetBuffer(bufHdr);- }+ TRACE_POSTGRESQL_BUFFER_READ_DONE(forkNum, blockNum,+ smgr->smgr_rlocator.locator.spcOid,+ smgr->smgr_rlocator.locator.dbOid,+ smgr->smgr_rlocator.locator.relNumber,+ smgr->smgr_rlocator.backend,+ found);
/*
- * We get here only in the corner case where we are trying to extend- * the relation but we found a pre-existing buffer marked BM_VALID.- * This can happen because mdread doesn't complain about reads beyond- * EOF (when zero_damaged_pages is ON) and so a previous attempt to- * read a block beyond EOF could have left a "valid" zero-filled- * buffer. Unfortunately, we have also seen this case occurring- * because of buggy Linux kernels that sometimes return an- * lseek(SEEK_END) result that doesn't account for a recent write. In- * that situation, the pre-existing buffer would contain valid data- * that we don't want to overwrite. Since the legitimate case should- * always have left a zero-filled buffer, complain if not PageIsNew.+ * In RBM_ZERO_AND_LOCK mode the caller expects the page to be+ * locked on return.
*/
- bufBlock = isLocalBuf ? LocalBufHdrGetBlock(bufHdr) : BufHdrGetBlock(bufHdr);- if (!PageIsNew((Page) bufBlock))- ereport(ERROR,- (errmsg("unexpected data beyond EOF in block %u of relation %s",- blockNum, relpath(smgr->smgr_rlocator, forkNum)),- errhint("This has been seen to occur with buggy kernels; consider updating your system.")));-- /*- * We *must* do smgrextend before succeeding, else the page will not- * be reserved by the kernel, and the next P_NEW call will decide to- * return the same page. Clear the BM_VALID bit, do the StartBufferIO- * call that BufferAlloc didn't, and proceed.- */- if (isLocalBuf)+ if (!isLocalBuf)
{
- /* Only need to adjust flags */- uint32 buf_state = pg_atomic_read_u32(&bufHdr->state);-- Assert(buf_state & BM_VALID);- buf_state &= ~BM_VALID;- pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state);+ if (mode == RBM_ZERO_AND_LOCK)+ LWLockAcquire(BufferDescriptorGetContentLock(bufHdr),+ LW_EXCLUSIVE);+ else if (mode == RBM_ZERO_AND_CLEANUP_LOCK)+ LockBufferForCleanup(BufferDescriptorGetBuffer(bufHdr));
}
- else- {- /*- * Loop to handle the very small possibility that someone re-sets- * BM_VALID between our clearing it and StartBufferIO inspecting- * it.- */- do- {- uint32 buf_state = LockBufHdr(bufHdr);- Assert(buf_state & BM_VALID);- buf_state &= ~BM_VALID;- UnlockBufHdr(bufHdr, buf_state);- } while (!StartBufferIO(bufHdr, true));- }+ return BufferDescriptorGetBuffer(bufHdr);
}
/*
* if we have gotten to this point, we have allocated a buffer for the
* page but its contents are not yet valid. IO_IN_PROGRESS is set for it,
* if it's a shared buffer.
- *- * Note: if smgrextend fails, we will end up with a buffer that is- * allocated but not marked BM_VALID. P_NEW will still select the same- * block number (because the relation didn't get any longer on disk) and- * so future attempts to extend the relation will find the same buffer (if- * it's not been recycled) but come right back here to try smgrextend- * again.
*/
Assert(!(pg_atomic_read_u32(&bufHdr->state) & BM_VALID)); /* spinlock not needed */
bufBlock = isLocalBuf ? LocalBufHdrGetBlock(bufHdr) : BufHdrGetBlock(bufHdr);
- if (isExtend)- {- /* new buffers are zero-filled */+ /*+ * Read in the page, unless the caller intends to overwrite it and+ * just wants us to allocate a buffer.+ */+ if (mode == RBM_ZERO_AND_LOCK || mode == RBM_ZERO_AND_CLEANUP_LOCK)
MemSet((char *) bufBlock, 0, BLCKSZ);
- /* don't set checksum for all-zero page */- smgrextend(smgr, forkNum, blockNum, (char *) bufBlock, false);-- /*- * NB: we're *not* doing a ScheduleBufferTagForWriteback here;- * although we're essentially performing a write. At least on linux- * doing so defeats the 'delayed allocation' mechanism, leading to- * increased file fragmentation.- */- }
else
{
- /*- * Read in the page, unless the caller intends to overwrite it and- * just wants us to allocate a buffer.- */- if (mode == RBM_ZERO_AND_LOCK || mode == RBM_ZERO_AND_CLEANUP_LOCK)- MemSet((char *) bufBlock, 0, BLCKSZ);- else+ instr_time io_start,+ io_time;++ if (track_io_timing)+ INSTR_TIME_SET_CURRENT(io_start);++ smgrread(smgr, forkNum, blockNum, (char *) bufBlock);++ if (track_io_timing)
{
- instr_time io_start,- io_time;+ INSTR_TIME_SET_CURRENT(io_time);+ INSTR_TIME_SUBTRACT(io_time, io_start);+ pgstat_count_buffer_read_time(INSTR_TIME_GET_MICROSEC(io_time));+ INSTR_TIME_ADD(pgBufferUsage.blk_read_time, io_time);+ }- if (track_io_timing)- INSTR_TIME_SET_CURRENT(io_start);-- smgrread(smgr, forkNum, blockNum, (char *) bufBlock);-- if (track_io_timing)+ /* check for garbage data */+ if (!PageIsVerifiedExtended((Page) bufBlock, blockNum,+ PIV_LOG_WARNING | PIV_REPORT_STAT))+ {+ if (mode == RBM_ZERO_ON_ERROR || zero_damaged_pages)
{
- INSTR_TIME_SET_CURRENT(io_time);- INSTR_TIME_SUBTRACT(io_time, io_start);- pgstat_count_buffer_read_time(INSTR_TIME_GET_MICROSEC(io_time));- INSTR_TIME_ADD(pgBufferUsage.blk_read_time, io_time);- }-- /* check for garbage data */- if (!PageIsVerifiedExtended((Page) bufBlock, blockNum,- PIV_LOG_WARNING | PIV_REPORT_STAT))- {- if (mode == RBM_ZERO_ON_ERROR || zero_damaged_pages)- {- ereport(WARNING,- (errcode(ERRCODE_DATA_CORRUPTED),- errmsg("invalid page in block %u of relation %s; zeroing out page",- blockNum,- relpath(smgr->smgr_rlocator, forkNum))));- MemSet((char *) bufBlock, 0, BLCKSZ);- }- else- ereport(ERROR,- (errcode(ERRCODE_DATA_CORRUPTED),- errmsg("invalid page in block %u of relation %s",- blockNum,- relpath(smgr->smgr_rlocator, forkNum))));+ ereport(WARNING,+ (errcode(ERRCODE_DATA_CORRUPTED),+ errmsg("invalid page in block %u of relation %s; zeroing out page",+ blockNum,+ relpath(smgr->smgr_rlocator, forkNum))));+ MemSet((char *) bufBlock, 0, BLCKSZ);
}
+ else+ ereport(ERROR,+ (errcode(ERRCODE_DATA_CORRUPTED),+ errmsg("invalid page in block %u of relation %s",+ blockNum,+ relpath(smgr->smgr_rlocator, forkNum))));
}
}
@@ -1074,7 +1058,6 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
smgr->smgr_rlocator.locator.dbOid,
smgr->smgr_rlocator.locator.relNumber,
smgr->smgr_rlocator.backend,
- isExtend,
found);
return BufferDescriptorGetBuffer(bufHdr);
@@ -1617,6 +1600,251 @@ again:
return cur_buf;
}
++/*+ * Limit the number of pins a batch operation may additionally acquire, to+ * avoid running out of pinnable buffers.+ *+ * One additional pin is always allowed, as otherwise the operation likely+ * cannot be performed at all.+ *+ * The number of allowed pins for a backend is computed based on+ * shared_buffers and the maximum number of connections possible. That's very+ * pessimistic, but oustide of toy-sized shared_buffers it should allow+ * sufficient pins.+ */+static void+LimitAdditionalPins(uint32 *additional_pins)+{+ uint32 max_backends;+ int max_proportional_pins;++ if (*additional_pins <= 1)+ return;++ max_backends = MaxBackends + NUM_AUXILIARY_PROCS;+ max_proportional_pins = NBuffers / max_backends;++ /*+ * Subtract the approximate number of buffers already pinned by this+ * backend. We get the number of "overflowed" pins for free, but don't+ * know the number of pins in PrivateRefCountArray. The cost of+ * calculating that exactly doesn't seem worth it, so just assume the max.+ */+ max_proportional_pins -= PrivateRefCountOverflowed + REFCOUNT_ARRAY_ENTRIES;++ if (max_proportional_pins < 0)+ max_proportional_pins = 1;++ if (*additional_pins > max_proportional_pins)+ *additional_pins = max_proportional_pins;+}++static BlockNumber+BulkExtendSharedRelationBuffered(Relation rel,+ SMgrRelation smgr,+ bool skip_extension_lock,+ char relpersistence,+ ForkNumber fork, ReadBufferMode mode,+ BufferAccessStrategy strategy,+ uint32 *num_pages,+ uint32 num_locked_pages,+ Buffer *buffers)+{+ BlockNumber first_block;++ LimitAdditionalPins(num_pages);++ /*+ * FIXME: limit num_pages / buffers based on NBuffers / MaxBackends or+ * such. Also keep MAX_SIMUL_LWLOCKS in mind.+ */++ pgBufferUsage.shared_blks_written += *num_pages;++ /*+ * Acquire victim buffers for extension without holding extension+ * lock. Writing out victim buffers is the most expensive part of+ * extending the relation, particularly when doing so requires WAL+ * flushes. Zeroing out the buffers is also quite expensive, so do that+ * before holding the extension lock as well.+ *+ * These pages are pinned by us and not valid. While we hold the pin+ * they can't be acquired as victim buffers by another backend.+ */+ for (uint32 i = 0; i < *num_pages; i++)+ {+ Block buf_block;++ buffers[i] = GetVictimBuffer(strategy);+ buf_block = BufHdrGetBlock(GetBufferDescriptor(buffers[i] - 1));++ /* new buffers are zero-filled */+ MemSet((char *) buf_block, 0, BLCKSZ);+ }++ if (!skip_extension_lock)+ LockRelationForExtension(rel, ExclusiveLock);++ first_block = smgrnblocks(smgr, fork);++ /* Fail if relation is already at maximum possible length */+ if ((uint64) first_block + *num_pages >= MaxBlockNumber)+ ereport(ERROR,+ (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),+ errmsg("cannot extend relation %s beyond %u blocks",+ relpath(smgr->smgr_rlocator, fork),+ MaxBlockNumber)));++ /*+ * Insert buffers into buffer table, mark as IO_IN_PROGRESS.+ *+ * This needs to happen before we extend the relation, because as soon as+ * we do, other backends can start to read in those pages.+ */+ for (int i = 0; i < *num_pages; i++)+ {+ Buffer victim_buf = buffers[i];+ BufferDesc *victim_buf_hdr = GetBufferDescriptor(victim_buf - 1);+ BufferTag tag;+ uint32 hash;+ LWLock *partition_lock;+ int existing_id;++ InitBufferTag(&tag, &smgr->smgr_rlocator.locator, fork, first_block + i);+ hash = BufTableHashCode(&tag);+ partition_lock = BufMappingPartitionLock(hash);++ LWLockAcquire(partition_lock, LW_EXCLUSIVE);++ existing_id = BufTableInsert(&tag, hash, victim_buf_hdr->buf_id);++ /*+ * We get here only in the corner case where we are trying to extend+ * the relation but we found a pre-existing buffer. This can happen+ * because a prior attempt at extending the relation failed, and+ * because mdread doesn't complain about reads beyond EOF (when+ * zero_damaged_pages is ON) and so a previous attempt to read a block+ * beyond EOF could have left a "valid" zero-filled buffer.+ * Unfortunately, we have also seen this case occurring because of+ * buggy Linux kernels that sometimes return an lseek(SEEK_END) result+ * that doesn't account for a recent write. In that situation, the+ * pre-existing buffer would contain valid data that we don't want to+ * overwrite. Since the legitimate cases should always have left a+ * zero-filled buffer, complain if not PageIsNew.+ */+ if (existing_id >= 0)+ {+ BufferDesc *existing_hdr = GetBufferDescriptor(existing_id);+ Block buf_block;+ bool valid;++ /*+ * Pin the existing buffer before releasing the partition lock,+ * preventing it from being evicted.+ */+ valid = PinBuffer(existing_hdr, strategy);++ LWLockRelease(partition_lock);++ /*+ * The victim buffer we acquired peviously is clean and unused,+ * let it be found again quickly+ */+ StrategyFreeBuffer(victim_buf_hdr);+ UnpinBuffer(victim_buf_hdr);++ buffers[i] = BufferDescriptorGetBuffer(existing_hdr);+ buf_block = BufHdrGetBlock(existing_hdr);++ if (valid && !PageIsNew((Page) buf_block))+ ereport(ERROR,+ (errmsg("unexpected data beyond EOF in block %u of relation %s",+ existing_hdr->tag.blockNum, relpath(smgr->smgr_rlocator, fork)),+ errhint("This has been seen to occur with buggy kernels; consider updating your system.")));++ /*+ * We *must* do smgr[zero]extend before succeeding, else the page+ * will not be reserved by the kernel, and the next P_NEW call+ * will decide to return the same page. Clear the BM_VALID bit,+ * do StartBufferIO() and proceed.+ *+ * Loop to handle the very small possibility that someone re-sets+ * BM_VALID between our clearing it and StartBufferIO inspecting+ * it.+ */+ do+ {+ uint32 buf_state = LockBufHdr(existing_hdr);++ buf_state &= ~BM_VALID;+ UnlockBufHdr(existing_hdr, buf_state);+ } while (!StartBufferIO(existing_hdr, true));+ }+ else+ {+ uint32 buf_state;++ buf_state = LockBufHdr(victim_buf_hdr);++ /* some sanity checks while we hold the buffer header lock */+ Assert(!(buf_state & (BM_VALID | BM_TAG_VALID | BM_DIRTY | BM_JUST_DIRTIED)));+ Assert(BUF_STATE_GET_REFCOUNT(buf_state) == 1);++ victim_buf_hdr->tag = tag;++ buf_state |= BM_TAG_VALID | BUF_USAGECOUNT_ONE;+ if (relpersistence == RELPERSISTENCE_PERMANENT || fork == INIT_FORKNUM)+ buf_state |= BM_PERMANENT;++ UnlockBufHdr(victim_buf_hdr, buf_state);++ LWLockRelease(partition_lock);++ /* XXX: could combine the locked operations in it with the above */+ StartBufferIO(victim_buf_hdr, true);+ }+ }++ /*+ * Note: if smgzerorextend fails, we will end up with buffers that are+ * allocated but not marked BM_VALID. The next relation extension will+ * still select the same block number (because the relation didn't get any+ * longer on disk) and so future attempts to extend the relation will find+ * the same buffers (if they have not been recycled) but come right back+ * here to try smgrzeroextend again.+ *+ * We don't need to set checksum for all-zero pages.+ */+ smgrzeroextend(smgr, fork, first_block, *num_pages, false);++ /*+ * Release the file-extension lock; it's now OK for someone else to extend+ * the relation some more.+ *+ * We remove IO_IN_PROGRESS after this, as zeroing the buffer contents and+ * waking up waiting backends waiting can take noticeable time.+ */+ if (!skip_extension_lock)+ UnlockRelationForExtension(rel, ExclusiveLock);++ /* Set BM_VALID, terminate IO, and wake up any waiters */+ for (int i = 0; i < *num_pages; i++)+ {+ Buffer buf = buffers[i];+ BufferDesc *buf_hdr = GetBufferDescriptor(buf - 1);++ if (i < num_locked_pages &&+ (mode == RBM_ZERO_AND_LOCK || mode == RBM_ZERO_AND_CLEANUP_LOCK))+ LWLockAcquire(BufferDescriptorGetContentLock(buf_hdr), LW_EXCLUSIVE);++ TerminateBufferIO(buf_hdr, false, BM_VALID);+ }++ return first_block;++}+
/*
* MarkBufferDirty
*
diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.cindex 6a3f325ea6d..5e93d8ba839 100644--- a/src/backend/storage/buffer/localbuf.c+++ b/src/backend/storage/buffer/localbuf.c@@ -48,6 +48,9 @@ static int nextFreeLocalBufId = 0;
static HTAB *LocalBufHash = NULL;
+/* number of local buffers pinned at least once */+static int NLocalPinnedBuffers = 0;+
static void InitLocalBuffers(void);
static Block GetLocalBufferStorage(void);
@@ -270,6 +273,132 @@ GetLocalVictimBuffer(void)
return BufferDescriptorGetBuffer(bufHdr);
}
+/* see LimitAdditionalPins() */+static void+LimitAdditionalLocalPins(uint32 *additional_pins)+{+ uint32 max_pins;++ if (*additional_pins <= 1)+ return;++ /*+ * In contrast to LimitAdditionalPins() other backends don't play a role+ * here. We can allow up to NLocBuffer pins in total.+ */+ max_pins = (NLocBuffer - NLocalPinnedBuffers);++ if (*additional_pins >= max_pins)+ *additional_pins = max_pins;+}++BlockNumber+BulkExtendLocalRelationBuffered(SMgrRelation smgr,+ ForkNumber fork,+ ReadBufferMode mode,+ uint32 *num_pages,+ Buffer *buffers)+{+ BlockNumber first_block;++ /* Initialize local buffers if first request in this session */+ if (LocalBufHash == NULL)+ InitLocalBuffers();++ LimitAdditionalLocalPins(num_pages);++ pgBufferUsage.temp_blks_written += *num_pages;++ for (uint32 i = 0; i < *num_pages; i++)+ {+ BufferDesc *buf_hdr;+ Block buf_block;++ buffers[i] = GetLocalVictimBuffer();+ buf_hdr = GetLocalBufferDescriptor(-(buffers[i] + 1));+ buf_block = LocalBufHdrGetBlock(buf_hdr);++ /* new buffers are zero-filled */+ MemSet((char *) buf_block, 0, BLCKSZ);+ }++ first_block = smgrnblocks(smgr, fork);++ /* Fail if relation is already at maximum possible length */+ if ((uint64) first_block + *num_pages >= MaxBlockNumber)+ ereport(ERROR,+ (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),+ errmsg("cannot extend relation %s beyond %u blocks",+ relpath(smgr->smgr_rlocator, fork),+ MaxBlockNumber)));++ for (int i = 0; i < *num_pages; i++)+ {+ int victim_buf_id;+ BufferDesc *victim_buf_hdr;+ BufferTag tag;+ LocalBufferLookupEnt *hresult;+ bool found;++ victim_buf_id = -(buffers[i] + 1);+ victim_buf_hdr = GetLocalBufferDescriptor(victim_buf_id);++ InitBufferTag(&tag, &smgr->smgr_rlocator.locator, fork, first_block + i);++ hresult = (LocalBufferLookupEnt *)+ hash_search(LocalBufHash, (void *) &tag, HASH_ENTER, &found);+ if (found)+ {+ BufferDesc *existing_hdr = GetLocalBufferDescriptor(hresult->id);+ uint32 buf_state;++ UnpinLocalBuffer(BufferDescriptorGetBuffer(victim_buf_hdr));++ existing_hdr = GetLocalBufferDescriptor(hresult->id);+ PinLocalBuffer(existing_hdr, false);+ buffers[i] = BufferDescriptorGetBuffer(existing_hdr);++ buf_state = pg_atomic_read_u32(&existing_hdr->state);+ Assert(buf_state & BM_TAG_VALID);+ Assert(!(buf_state & BM_DIRTY));+ buf_state &= BM_VALID;+ pg_atomic_unlocked_write_u32(&existing_hdr->state, buf_state);+ }+ else+ {+ uint32 buf_state = pg_atomic_read_u32(&victim_buf_hdr->state);++ Assert(!(buf_state & (BM_VALID | BM_TAG_VALID | BM_DIRTY | BM_JUST_DIRTIED)));++ victim_buf_hdr->tag = tag;++ buf_state |= BM_TAG_VALID | BUF_USAGECOUNT_ONE;++ pg_atomic_unlocked_write_u32(&victim_buf_hdr->state, buf_state);++ hresult->id = victim_buf_id;+ }+ }++ /* actually extend relation */+ smgrzeroextend(smgr, fork, first_block, *num_pages, false);++ for (int i = 0; i < *num_pages; i++)+ {+ Buffer buf = buffers[i];+ BufferDesc *buf_hdr;+ uint32 buf_state;++ buf_hdr = GetLocalBufferDescriptor(-(buf + 1));++ buf_state = pg_atomic_read_u32(&buf_hdr->state);+ buf_state |= BM_VALID;+ pg_atomic_unlocked_write_u32(&buf_hdr->state, buf_state);+ }++ return first_block;+}+
/*
* MarkLocalBufferDirty -
* mark a local buffer dirty
@@ -486,6 +615,7 @@ PinLocalBuffer(BufferDesc *buf_hdr, bool adjust_usagecount)
if (LocalRefCount[bufid] == 0)
{
+ NLocalPinnedBuffers++;
if (adjust_usagecount &&
BUF_STATE_GET_USAGECOUNT(buf_state) < BM_MAX_USAGE_COUNT)
{
@@ -507,9 +637,11 @@ UnpinLocalBuffer(Buffer buffer)
Assert(BufferIsLocal(buffer));
Assert(LocalRefCount[buffid] > 0);
+ Assert(NLocalPinnedBuffers > 0);
ResourceOwnerForgetBuffer(CurrentResourceOwner, buffer);
- LocalRefCount[buffid]--;+ if (--LocalRefCount[buffid] == 0)+ NLocalPinnedBuffers--;
}
/*
diff --git a/src/backend/utils/probes.d b/src/backend/utils/probes.dindex 3ebbcf88ebe..12b4d995cfa 100644--- a/src/backend/utils/probes.d+++ b/src/backend/utils/probes.d@@ -55,10 +55,12 @@ provider postgresql {
probe sort__start(int, bool, int, int, bool, int);
probe sort__done(bool, long);
- probe buffer__read__start(ForkNumber, BlockNumber, Oid, Oid, Oid, int, bool);- probe buffer__read__done(ForkNumber, BlockNumber, Oid, Oid, Oid, int, bool, bool);+ probe buffer__read__start(ForkNumber, BlockNumber, Oid, Oid, Oid, int);+ probe buffer__read__done(ForkNumber, BlockNumber, Oid, Oid, Oid, int, bool);
probe buffer__flush__start(ForkNumber, BlockNumber, Oid, Oid, Oid);
probe buffer__flush__done(ForkNumber, BlockNumber, Oid, Oid, Oid);
+ probe buffer__extend__start(ForkNumber, Oid, Oid, Oid, int, int);+ probe buffer__extend__done(ForkNumber, Oid, Oid, Oid, int, int, BlockNumber);
probe buffer__checkpoint__start(int);
probe buffer__checkpoint__sync__start();
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgmlindex e5d622d5147..b58a30ef5a8 100644--- a/doc/src/sgml/monitoring.sgml+++ b/doc/src/sgml/monitoring.sgml@@ -7344,7 +7344,7 @@ FROM pg_stat_get_backend_idset() AS backendid;
</row>
<row>
<entry><literal>buffer-read-start</literal></entry>
- <entry><literal>(ForkNumber, BlockNumber, Oid, Oid, Oid, int, bool)</literal></entry>+ <entry><literal>(ForkNumber, BlockNumber, Oid, Oid, Oid, int)</literal></entry>
<entry>Probe that fires when a buffer read is started.
arg0 and arg1 contain the fork and block numbers of the page (but
arg1 will be -1 if this is a relation extension request).
@@ -7352,12 +7352,11 @@ FROM pg_stat_get_backend_idset() AS backendid;
identifying the relation.
arg5 is the ID of the backend which created the temporary relation for a
local buffer, or <symbol>InvalidBackendId</symbol> (-1) for a shared buffer.
- arg6 is true for a relation extension request, false for normal- read.</entry>+ </entry>
</row>
<row>
<entry><literal>buffer-read-done</literal></entry>
- <entry><literal>(ForkNumber, BlockNumber, Oid, Oid, Oid, int, bool, bool)</literal></entry>+ <entry><literal>(ForkNumber, BlockNumber, Oid, Oid, Oid, int, bool)</literal></entry>
<entry>Probe that fires when a buffer read is complete.
arg0 and arg1 contain the fork and block numbers of the page (if this
is a relation extension request, arg1 now contains the block number
@@ -7366,9 +7365,7 @@ FROM pg_stat_get_backend_idset() AS backendid;
identifying the relation.
arg5 is the ID of the backend which created the temporary relation for a
local buffer, or <symbol>InvalidBackendId</symbol> (-1) for a shared buffer.
- arg6 is true for a relation extension request, false for normal- read.- arg7 is true if the buffer was found in the pool, false if not.</entry>+ arg6 is true if the buffer was found in the pool, false if not.</entry>
</row>
<row>
<entry><literal>buffer-flush-start</literal></entry>
--
2.38.0
[text/x-diff] v1-0009-Convert-a-few-places-to-ExtendRelationBuffered.patch (13.5K, ../20221029025420.eplyow6k7tgu6he3@awork3.anarazel.de/10-v1-0009-Convert-a-few-places-to-ExtendRelationBuffered.patch)
download | inline diff:
From 7a6abff9eb31cf812fdfa4804a853091a27c38e7 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Mon, 24 Oct 2022 12:18:18 -0700
Subject: [PATCH v1 09/12] Convert a few places to ExtendRelationBuffered
Author:
Reviewed-by:
Discussion: https://postgr.es/m/
Backpatch:
---
src/backend/access/brin/brin.c | 13 ++++++----
src/backend/access/brin/brin_pageops.c | 4 +++
src/backend/access/brin/brin_revmap.c | 17 ++++--------
src/backend/access/gin/gininsert.c | 14 +++++-----
src/backend/access/gin/ginutil.c | 15 +++--------
src/backend/access/gin/ginvacuum.c | 8 ++++++
src/backend/access/gist/gist.c | 6 +++--
src/backend/access/gist/gistutil.c | 16 +++---------
src/backend/access/gist/gistvacuum.c | 3 +++
src/backend/access/nbtree/nbtpage.c | 36 ++++++++------------------
src/backend/access/nbtree/nbtree.c | 3 +++
src/backend/access/spgist/spgutils.c | 15 +++--------
contrib/bloom/blutils.c | 14 +++-------
13 files changed, 70 insertions(+), 94 deletions(-)
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.cindex 20b7d65b948..27266b84b0f 100644--- a/src/backend/access/brin/brin.c+++ b/src/backend/access/brin/brin.c@@ -829,9 +829,11 @@ brinbuild(Relation heap, Relation index, IndexInfo *indexInfo)
* whole relation will be rolled back.
*/
- meta = ReadBuffer(index, P_NEW);+ meta = ExtendRelationBuffered(index, NULL, true,+ index->rd_rel->relpersistence,+ MAIN_FORKNUM, RBM_ZERO_AND_LOCK,+ NULL);
Assert(BufferGetBlockNumber(meta) == BRIN_METAPAGE_BLKNO);
- LockBuffer(meta, BUFFER_LOCK_EXCLUSIVE);
brin_metapage_init(BufferGetPage(meta), BrinGetPagesPerRange(index),
BRIN_CURRENT_VERSION);
@@ -896,9 +898,10 @@ brinbuildempty(Relation index)
Buffer metabuf;
/* An empty BRIN index has a metapage only. */
- metabuf =- ReadBufferExtended(index, INIT_FORKNUM, P_NEW, RBM_NORMAL, NULL);- LockBuffer(metabuf, BUFFER_LOCK_EXCLUSIVE);+ metabuf = ExtendRelationBuffered(index, NULL, true,+ index->rd_rel->relpersistence,+ INIT_FORKNUM, RBM_ZERO_AND_LOCK,+ NULL);
/* Initialize and xlog metabuffer. */
START_CRIT_SECTION();
diff --git a/src/backend/access/brin/brin_pageops.c b/src/backend/access/brin/brin_pageops.cindex f17aad51b63..8525d6be918 100644--- a/src/backend/access/brin/brin_pageops.c+++ b/src/backend/access/brin/brin_pageops.c@@ -730,6 +730,10 @@ brin_getinsertbuffer(Relation irel, Buffer oldbuf, Size itemsz,
* There's not enough free space in any existing index page,
* according to the FSM: extend the relation to obtain a shiny new
* page.
+ *+ * XXX: It's likely possible to use RBM_ZERO_AND_LOCK here,+ * which'd avoid the need to hold the extension lock during buffer+ * reclaim.
*/
if (!RELATION_IS_LOCAL(irel))
{
diff --git a/src/backend/access/brin/brin_revmap.c b/src/backend/access/brin/brin_revmap.cindex 6e392a551ad..8b7c72f38d3 100644--- a/src/backend/access/brin/brin_revmap.c+++ b/src/backend/access/brin/brin_revmap.c@@ -538,7 +538,6 @@ revmap_physical_extend(BrinRevmap *revmap)
BlockNumber mapBlk;
BlockNumber nblocks;
Relation irel = revmap->rm_irel;
- bool needLock = !RELATION_IS_LOCAL(irel);
/*
* Lock the metapage. This locks out concurrent extensions of the revmap,
@@ -570,10 +569,10 @@ revmap_physical_extend(BrinRevmap *revmap)
}
else
{
- if (needLock)- LockRelationForExtension(irel, ExclusiveLock);-- buf = ReadBuffer(irel, P_NEW);+ buf = ExtendRelationBuffered(irel, NULL, false,+ irel->rd_rel->relpersistence,+ MAIN_FORKNUM, RBM_ZERO_AND_LOCK,+ NULL);
if (BufferGetBlockNumber(buf) != mapBlk)
{
/*
@@ -582,17 +581,11 @@ revmap_physical_extend(BrinRevmap *revmap)
* up and have caller start over. We will have to evacuate that
* page from under whoever is using it.
*/
- if (needLock)- UnlockRelationForExtension(irel, ExclusiveLock);
LockBuffer(revmap->rm_metaBuf, BUFFER_LOCK_UNLOCK);
- ReleaseBuffer(buf);+ UnlockReleaseBuffer(buf);
return;
}
- LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
page = BufferGetPage(buf);
-- if (needLock)- UnlockRelationForExtension(irel, ExclusiveLock);
}
/* Check that it's a regular block (or an empty page) */
diff --git a/src/backend/access/gin/gininsert.c b/src/backend/access/gin/gininsert.cindex ea1c4184fbf..795efa199b0 100644--- a/src/backend/access/gin/gininsert.c+++ b/src/backend/access/gin/gininsert.c@@ -440,12 +440,14 @@ ginbuildempty(Relation index)
MetaBuffer;
/* An empty GIN index has two pages. */
- MetaBuffer =- ReadBufferExtended(index, INIT_FORKNUM, P_NEW, RBM_NORMAL, NULL);- LockBuffer(MetaBuffer, BUFFER_LOCK_EXCLUSIVE);- RootBuffer =- ReadBufferExtended(index, INIT_FORKNUM, P_NEW, RBM_NORMAL, NULL);- LockBuffer(RootBuffer, BUFFER_LOCK_EXCLUSIVE);+ MetaBuffer = ExtendRelationBuffered(index, NULL, true,+ index->rd_rel->relpersistence,+ INIT_FORKNUM, RBM_ZERO_AND_LOCK,+ NULL);+ RootBuffer = ExtendRelationBuffered(index, NULL, true,+ index->rd_rel->relpersistence,+ INIT_FORKNUM, RBM_ZERO_AND_LOCK,+ NULL);
/* Initialize and xlog metabuffer and root buffer. */
START_CRIT_SECTION();
diff --git a/src/backend/access/gin/ginutil.c b/src/backend/access/gin/ginutil.cindex 6df7f2eaeba..12a7b6d03e1 100644--- a/src/backend/access/gin/ginutil.c+++ b/src/backend/access/gin/ginutil.c@@ -298,7 +298,6 @@ Buffer
GinNewBuffer(Relation index)
{
Buffer buffer;
- bool needLock;
/* First, try to get a page from FSM */
for (;;)
@@ -326,16 +325,10 @@ GinNewBuffer(Relation index)
ReleaseBuffer(buffer);
}
- /* Must extend the file */- needLock = !RELATION_IS_LOCAL(index);- if (needLock)- LockRelationForExtension(index, ExclusiveLock);-- buffer = ReadBuffer(index, P_NEW);- LockBuffer(buffer, GIN_EXCLUSIVE);-- if (needLock)- UnlockRelationForExtension(index, ExclusiveLock);+ buffer = ExtendRelationBuffered(index, NULL, false,+ index->rd_rel->relpersistence,+ MAIN_FORKNUM, RBM_ZERO_AND_LOCK,+ NULL);
return buffer;
}
diff --git a/src/backend/access/gin/ginvacuum.c b/src/backend/access/gin/ginvacuum.cindex b4fa5f6bf81..8ec7b399287 100644--- a/src/backend/access/gin/ginvacuum.c+++ b/src/backend/access/gin/ginvacuum.c@@ -736,6 +736,10 @@ ginvacuumcleanup(IndexVacuumInfo *info, IndexBulkDeleteResult *stats)
*/
needLock = !RELATION_IS_LOCAL(index);
+ /*+ * FIXME: Now that new pages are locked with RBM_ZERO_AND_LOCK, I don't+ * think this is still required?+ */
if (needLock)
LockRelationForExtension(index, ExclusiveLock);
npages = RelationGetNumberOfBlocks(index);
@@ -786,6 +790,10 @@ ginvacuumcleanup(IndexVacuumInfo *info, IndexBulkDeleteResult *stats)
stats->pages_free = totFreePages;
+ /*+ * FIXME: Now that new pages are locked with RBM_ZERO_AND_LOCK, I don't+ * think this is still required?+ */
if (needLock)
LockRelationForExtension(index, ExclusiveLock);
stats->num_pages = RelationGetNumberOfBlocks(index);
diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.cindex 30069f139c7..014abdc53c0 100644--- a/src/backend/access/gist/gist.c+++ b/src/backend/access/gist/gist.c@@ -133,8 +133,10 @@ gistbuildempty(Relation index)
Buffer buffer;
/* Initialize the root page */
- buffer = ReadBufferExtended(index, INIT_FORKNUM, P_NEW, RBM_NORMAL, NULL);- LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);+ buffer = ExtendRelationBuffered(index, NULL, true,+ index->rd_rel->relpersistence,+ INIT_FORKNUM, RBM_ZERO_AND_LOCK,+ NULL);
/* Initialize and xlog buffer */
START_CRIT_SECTION();
diff --git a/src/backend/access/gist/gistutil.c b/src/backend/access/gist/gistutil.cindex 1532462317b..22d8c49b3f7 100644--- a/src/backend/access/gist/gistutil.c+++ b/src/backend/access/gist/gistutil.c@@ -824,7 +824,6 @@ Buffer
gistNewBuffer(Relation r)
{
Buffer buffer;
- bool needLock;
/* First, try to get a page from FSM */
for (;;)
@@ -877,17 +876,10 @@ gistNewBuffer(Relation r)
ReleaseBuffer(buffer);
}
- /* Must extend the file */- needLock = !RELATION_IS_LOCAL(r);-- if (needLock)- LockRelationForExtension(r, ExclusiveLock);-- buffer = ReadBuffer(r, P_NEW);- LockBuffer(buffer, GIST_EXCLUSIVE);-- if (needLock)- UnlockRelationForExtension(r, ExclusiveLock);+ buffer = ExtendRelationBuffered(r, NULL, false,+ r->rd_rel->relpersistence,+ MAIN_FORKNUM, RBM_ZERO_AND_LOCK,+ NULL);
return buffer;
}
diff --git a/src/backend/access/gist/gistvacuum.c b/src/backend/access/gist/gistvacuum.cindex 0aa6e58a62f..d19afc1e4b1 100644--- a/src/backend/access/gist/gistvacuum.c+++ b/src/backend/access/gist/gistvacuum.c@@ -203,6 +203,9 @@ gistvacuumscan(IndexVacuumInfo *info, IndexBulkDeleteResult *stats,
* we must already have processed any tuples due to be moved into such a
* page.
*
+ * FIXME: Now that new pages are locked with RBM_ZERO_AND_LOCK, I don't+ * think this issue still exists?+ *
* We can skip locking for new or temp relations, however, since no one
* else could be accessing them.
*/
diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.cindex 8b96708b3ea..b4367b73630 100644--- a/src/backend/access/nbtree/nbtpage.c+++ b/src/backend/access/nbtree/nbtpage.c@@ -881,7 +881,6 @@ _bt_getbuf(Relation rel, BlockNumber blkno, int access)
}
else
{
- bool needLock;
Page page;
Assert(access == BT_WRITE);
@@ -962,31 +961,18 @@ _bt_getbuf(Relation rel, BlockNumber blkno, int access)
}
/*
- * Extend the relation by one page.- *- * We have to use a lock to ensure no one else is extending the rel at- * the same time, else we will both try to initialize the same new- * page. We can skip locking for new or temp relations, however,- * since no one else could be accessing them.+ * Extend the relation by one page. Need to use RBM_ZERO_AND_LOCK or+ * we risk a race condition against btvacuumscan --- see comments+ * therein. This forces us to repeat the valgrind request that+ * _bt_lockbuf() otherwise would make, as we can't use _bt_lockbuf()+ * without introducing a race.
*/
- needLock = !RELATION_IS_LOCAL(rel);-- if (needLock)- LockRelationForExtension(rel, ExclusiveLock);-- buf = ReadBuffer(rel, P_NEW);-- /* Acquire buffer lock on new page */- _bt_lockbuf(rel, buf, BT_WRITE);-- /*- * Release the file-extension lock; it's now OK for someone else to- * extend the relation some more. Note that we cannot release this- * lock before we have buffer lock on the new page, or we risk a race- * condition against btvacuumscan --- see comments therein.- */- if (needLock)- UnlockRelationForExtension(rel, ExclusiveLock);+ buf = ExtendRelationBuffered(rel, NULL, false,+ rel->rd_rel->relpersistence,+ MAIN_FORKNUM, RBM_ZERO_AND_LOCK,+ NULL);+ if (!RelationUsesLocalBuffers(rel))+ VALGRIND_MAKE_MEM_DEFINED(BufferGetPage(buf), BLCKSZ);
/* Initialize the new page before returning it */
page = BufferGetPage(buf);
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.cindex b52eca8f38b..904ec32354f 100644--- a/src/backend/access/nbtree/nbtree.c+++ b/src/backend/access/nbtree/nbtree.c@@ -969,6 +969,9 @@ btvacuumscan(IndexVacuumInfo *info, IndexBulkDeleteResult *stats,
* write-lock on the left page before it adds a right page, so we must
* already have processed any tuples due to be moved into such a page.
*
+ * FIXME: Now that new pages are locked with RBM_ZERO_AND_LOCK, I don't+ * think this issue still exists?+ *
* We can skip locking for new or temp relations, however, since no one
* else could be accessing them.
*/
diff --git a/src/backend/access/spgist/spgutils.c b/src/backend/access/spgist/spgutils.cindex 2c661fcf96f..bd0993a0501 100644--- a/src/backend/access/spgist/spgutils.c+++ b/src/backend/access/spgist/spgutils.c@@ -365,7 +365,6 @@ Buffer
SpGistNewBuffer(Relation index)
{
Buffer buffer;
- bool needLock;
/* First, try to get a page from FSM */
for (;;)
@@ -405,16 +404,10 @@ SpGistNewBuffer(Relation index)
ReleaseBuffer(buffer);
}
- /* Must extend the file */- needLock = !RELATION_IS_LOCAL(index);- if (needLock)- LockRelationForExtension(index, ExclusiveLock);-- buffer = ReadBuffer(index, P_NEW);- LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);-- if (needLock)- UnlockRelationForExtension(index, ExclusiveLock);+ buffer = ExtendRelationBuffered(index, NULL, false,+ index->rd_rel->relpersistence,+ MAIN_FORKNUM, RBM_ZERO_AND_LOCK,+ NULL);
return buffer;
}
diff --git a/contrib/bloom/blutils.c b/contrib/bloom/blutils.cindex a434cf93efd..39b5ad47e18 100644--- a/contrib/bloom/blutils.c+++ b/contrib/bloom/blutils.c@@ -353,7 +353,6 @@ Buffer
BloomNewBuffer(Relation index)
{
Buffer buffer;
- bool needLock;
/* First, try to get a page from FSM */
for (;;)
@@ -387,15 +386,10 @@ BloomNewBuffer(Relation index)
}
/* Must extend the file */
- needLock = !RELATION_IS_LOCAL(index);- if (needLock)- LockRelationForExtension(index, ExclusiveLock);-- buffer = ReadBuffer(index, P_NEW);- LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);-- if (needLock)- UnlockRelationForExtension(index, ExclusiveLock);+ buffer = ExtendRelationBuffered(index, NULL, false,+ index->rd_rel->relpersistence,+ MAIN_FORKNUM, RBM_ZERO_AND_LOCK,+ NULL);
return buffer;
}
--
2.38.0
[text/x-diff] v1-0010-heapam-Add-num_pages-to-RelationGetBufferForTuple.patch (6.8K, ../20221029025420.eplyow6k7tgu6he3@awork3.anarazel.de/11-v1-0010-heapam-Add-num_pages-to-RelationGetBufferForTuple.patch)
download | inline diff:
From 9882dcba636ce45443b0a377efbc87d8a069853d Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Sun, 23 Oct 2022 14:44:43 -0700
Subject: [PATCH v1 10/12] heapam: Add num_pages to RelationGetBufferForTuple()
This will be useful to compute the number of pages to extend a relation by.
Author:
Reviewed-by:
Discussion: https://postgr.es/m/
Backpatch:
---
src/include/access/hio.h | 14 +++++++-
src/backend/access/heap/heapam.c | 60 +++++++++++++++++++++++++++++---
src/backend/access/heap/hio.c | 8 ++++-
3 files changed, 76 insertions(+), 6 deletions(-)
diff --git a/src/include/access/hio.h b/src/include/access/hio.hindex bb90c6fad81..a7326e28d9a 100644--- a/src/include/access/hio.h+++ b/src/include/access/hio.h@@ -30,6 +30,17 @@ typedef struct BulkInsertStateData
{
BufferAccessStrategy strategy; /* our BULKWRITE strategy object */
Buffer current_buf; /* current insertion target page */
++ /*+ * State for bulk extensions. Further pages that were unused at the time+ * of the extension. They might be in use by the time we use them though,+ * so rechecks are needed.+ *+ * FIXME: Perhaps these should live in RelationData instead, alongside the+ * targetblock?+ */+ BlockNumber next_free;+ BlockNumber last_free;
} BulkInsertStateData;
@@ -38,6 +49,7 @@ extern void RelationPutHeapTuple(Relation relation, Buffer buffer,
extern Buffer RelationGetBufferForTuple(Relation relation, Size len,
Buffer otherBuffer, int options,
BulkInsertStateData *bistate,
- Buffer *vmbuffer, Buffer *vmbuffer_other);+ Buffer *vmbuffer, Buffer *vmbuffer_other,+ int num_pages);
#endif /* HIO_H */
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.cindex bd4d85041d3..51e3a9fff34 100644--- a/src/backend/access/heap/heapam.c+++ b/src/backend/access/heap/heapam.c@@ -1977,6 +1977,8 @@ GetBulkInsertState(void)
bistate = (BulkInsertState) palloc(sizeof(BulkInsertStateData));
bistate->strategy = GetAccessStrategy(BAS_BULKWRITE);
bistate->current_buf = InvalidBuffer;
+ bistate->next_free = InvalidBlockNumber;+ bistate->last_free = InvalidBlockNumber;
return bistate;
}
@@ -2050,7 +2052,8 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
*/
buffer = RelationGetBufferForTuple(relation, heaptup->t_len,
InvalidBuffer, options, bistate,
- &vmbuffer, NULL);+ &vmbuffer, NULL,+ 0);
/*
* We're about to do the actual insert -- but check for conflict first, to
@@ -2253,6 +2256,33 @@ heap_prepare_insert(Relation relation, HeapTuple tup, TransactionId xid,
return tup;
}
+/*+ * Helper for heap_multi_insert() that computes the number of full pages s+ */+static int+heap_multi_insert_pages(HeapTuple *heaptuples, int done, int ntuples, Size saveFreeSpace)+{+ size_t page_avail;+ int npages = 0;++ page_avail = BLCKSZ - SizeOfPageHeaderData - saveFreeSpace;+ npages++;++ for (int i = done; i < ntuples; i++)+ {+ size_t tup_sz = sizeof(ItemIdData) + MAXALIGN(heaptuples[i]->t_len);++ if (page_avail < tup_sz)+ {+ npages++;+ page_avail = BLCKSZ - SizeOfPageHeaderData - saveFreeSpace;+ }+ page_avail -= tup_sz;+ }++ return npages;+}+
/*
* heap_multi_insert - insert multiple tuples into a heap
*
@@ -2279,6 +2309,9 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
Size saveFreeSpace;
bool need_tuple_data = RelationIsLogicallyLogged(relation);
bool need_cids = RelationIsAccessibleInLogicalDecoding(relation);
+ bool starting_with_empty_page = false;+ int npages = 0;+ int npages_used = 0;
/* currently not needed (thus unsupported) for heap_multi_insert() */
AssertArg(!(options & HEAP_INSERT_NO_LOGICAL));
@@ -2329,13 +2362,30 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
while (ndone < ntuples)
{
Buffer buffer;
- bool starting_with_empty_page;
bool all_visible_cleared = false;
bool all_frozen_set = false;
int nthispage;
CHECK_FOR_INTERRUPTS();
+ /*+ * Compute number of pages needed to insert tuples in the worst+ * case. This will be used to determine how much to extend the+ * relation by in RelationGetBufferForTuple(), if needed. If we+ * filled a prior page from scratch, we can just update our last+ * computation, but if we started with a partially filled page+ * recompute from scratch, the number of potentially required pages+ * can vary due to tuples needing to fit onto the page, page headers+ * etc.+ */+ if (ndone == 0 || !starting_with_empty_page)+ {+ npages = heap_multi_insert_pages(heaptuples, ndone, ntuples, saveFreeSpace);+ npages_used = 0;+ }+ else+ npages_used++;+
/*
* Find buffer where at least the next tuple will fit. If the page is
* all-visible, this will also pin the requisite visibility map page.
@@ -2345,7 +2395,8 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
*/
buffer = RelationGetBufferForTuple(relation, heaptuples[ndone]->t_len,
InvalidBuffer, options, bistate,
- &vmbuffer, NULL);+ &vmbuffer, NULL,+ npages - npages_used);
page = BufferGetPage(buffer);
starting_with_empty_page = PageGetMaxOffsetNumber(page) == 0;
@@ -3768,7 +3819,8 @@ l2:
/* It doesn't fit, must use RelationGetBufferForTuple. */
newbuf = RelationGetBufferForTuple(relation, heaptup->t_len,
buffer, 0, NULL,
- &vmbuffer_new, &vmbuffer);+ &vmbuffer_new, &vmbuffer,+ 0);
/* We're all done. */
break;
}
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.cindex bc4035a6221..0d43ebb3877 100644--- a/src/backend/access/heap/hio.c+++ b/src/backend/access/heap/hio.c@@ -275,6 +275,11 @@ RelationAddExtraBlocks(Relation relation, BulkInsertState bistate)
* Returns pinned and exclusive-locked buffer of a page in given relation
* with free space >= given len.
*
+ * If num_pages is > 1, the relation will be extended by at least that many+ * pages when we decide to extend the relation. This is more efficient for+ * callers that know they will need multiple pages+ * (e.g. heap_multi_insert()).+ *
* If otherBuffer is not InvalidBuffer, then it references a previously
* pinned buffer of another page in the same relation; on return, this
* buffer will also be exclusive-locked. (This case is used by heap_update;
@@ -333,7 +338,8 @@ Buffer
RelationGetBufferForTuple(Relation relation, Size len,
Buffer otherBuffer, int options,
BulkInsertState bistate,
- Buffer *vmbuffer, Buffer *vmbuffer_other)+ Buffer *vmbuffer, Buffer *vmbuffer_other,+ int num_pages)
{
bool use_fsm = !(options & HEAP_INSERT_SKIP_FSM);
Buffer buffer = InvalidBuffer;
--
2.38.0
[text/x-diff] v1-0011-hio-Use-BulkExtendRelationBuffered.patch (7.0K, ../20221029025420.eplyow6k7tgu6he3@awork3.anarazel.de/12-v1-0011-hio-Use-BulkExtendRelationBuffered.patch)
download | inline diff:
From 23d7d9173e12f7105047c424836f57347c8fb15f Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Wed, 26 Oct 2022 14:14:11 -0700
Subject: [PATCH v1 11/12] hio: Use BulkExtendRelationBuffered()
---
src/backend/access/heap/hio.c | 181 +++++++++++++++++++++++++++++++---
1 file changed, 170 insertions(+), 11 deletions(-)
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.cindex 0d43ebb3877..51fadc9ac4c 100644--- a/src/backend/access/heap/hio.c+++ b/src/backend/access/heap/hio.c@@ -24,6 +24,7 @@
#include "storage/lmgr.h"
#include "storage/smgr.h"
+#define NEW_EXTEND
/*
* RelationPutHeapTuple - place tuple at specified page
@@ -185,6 +186,8 @@ GetVisibilityMapPins(Relation relation, Buffer buffer1, Buffer buffer2,
}
}
+#ifndef NEW_EXTEND+
/*
* Extend a relation by multiple blocks to avoid future contention on the
* relation extension lock. Our goal is to pre-extend the relation by an
@@ -268,6 +271,7 @@ RelationAddExtraBlocks(Relation relation, BulkInsertState bistate)
*/
FreeSpaceMapVacuumRange(relation, firstBlock, blockNum + 1);
}
+#endif
/*
* RelationGetBufferForTuple
@@ -354,6 +358,9 @@ RelationGetBufferForTuple(Relation relation, Size len,
len = MAXALIGN(len); /* be conservative */
+ if (num_pages <= 0)+ num_pages = 1;+
/* Bulk insert is not supported for updates, only inserts. */
Assert(otherBuffer == InvalidBuffer || !bistate);
@@ -558,18 +565,46 @@ loop:
ReleaseBuffer(buffer);
}
- /* Without FSM, always fall out of the loop and extend */- if (!use_fsm)- break;+ if (bistate+ && bistate->next_free != InvalidBlockNumber+ && bistate->next_free <= bistate->last_free)+ {+ /*+ * We bulk extended the relation before, and there are still some+ * unused pages from that extension, so we don't need to look in+ * the FSM for a new page. But do record the free space from the+ * last page, somebody might insert narrower tuples later.+ */+ if (use_fsm)+ RecordPageWithFreeSpace(relation, targetBlock, pageFreeSpace);- /*- * Update FSM as to condition of this page, and ask for another page- * to try.- */- targetBlock = RecordAndGetPageWithFreeSpace(relation,- targetBlock,- pageFreeSpace,- targetFreeSpace);+ Assert(bistate->last_free != InvalidBlockNumber &&+ bistate->next_free <= bistate->last_free);+ targetBlock = bistate->next_free;+ if (bistate->next_free >= bistate->last_free)+ {+ bistate->next_free = InvalidBlockNumber;+ bistate->last_free = InvalidBlockNumber;+ }+ else+ bistate->next_free++;+ }+ else if (!use_fsm)+ {+ /* Without FSM, always fall out of the loop and extend */+ break;+ }+ else+ {+ /*+ * Update FSM as to condition of this page, and ask for another page+ * to try.+ */+ targetBlock = RecordAndGetPageWithFreeSpace(relation,+ targetBlock,+ pageFreeSpace,+ targetFreeSpace);+ }
}
/*
@@ -582,6 +617,129 @@ loop:
*/
needLock = !RELATION_IS_LOCAL(relation);
+#ifdef NEW_EXTEND+ {+#define MAX_BUFFERS 64+ Buffer victim_buffers[MAX_BUFFERS];+ BlockNumber firstBlock = InvalidBlockNumber;+ BlockNumber firstBlockFSM = InvalidBlockNumber;+ BlockNumber curBlock;+ uint32 extend_by_pages;+ uint32 no_fsm_pages;+ uint32 waitcount;++ extend_by_pages = num_pages;++ /*+ * Multiply the number of pages to extend by the number of waiters. Do+ * this even if we're not using the FSM, as it does relieve+ * contention. Pages will be found via bistate->next_free.+ */+ if (needLock)+ waitcount = RelationExtensionLockWaiterCount(relation);+ else+ waitcount = 0;+ extend_by_pages += extend_by_pages * waitcount;++ /*+ * can't extend by more than MAX_BUFFERS, we need to pin them all+ * concurrently. FIXME: Need an NBuffers / MaxBackends type limit+ * here.+ */+ extend_by_pages = Min(extend_by_pages, MAX_BUFFERS);++ /*+ * How many of the extended pages not to enter into the FSM.+ *+ * Only enter pages that we don't need ourselves into the+ * FSM. Otherwise every other backend will immediately try to use the+ * pages this backend neds itself, causing unnecessary contention.+ *+ * Bulk extended pages are remembered in bistate->next_free_buffer. So+ * without a bistate we can't directly make use of them.+ *+ * Never enter the page returned into the FSM, we'll immediately use+ * it.+ */+ if (num_pages > 1 && bistate == NULL)+ no_fsm_pages = 1;+ else+ no_fsm_pages = num_pages;++ if (bistate && bistate->current_buf != InvalidBuffer)+ {+ ReleaseBuffer(bistate->current_buf);+ bistate->current_buf = InvalidBuffer;+ }++ firstBlock = BulkExtendRelationBuffered(relation,+ NULL,+ false,+ relation->rd_rel->relpersistence,+ MAIN_FORKNUM,+ RBM_ZERO_AND_LOCK,+ bistate ? bistate->strategy : NULL,+ &extend_by_pages,+ 1,+ victim_buffers);+ /*+ * Relation is now extended. Make all but the first buffer available+ * to other backends.+ *+ * XXX: We don't necessarily need to release pin / update FSM while+ * holding the extension lock. But there are some advantages.+ */+ curBlock = firstBlock;+ for (uint32 i = 0; i < extend_by_pages; i++, curBlock++)+ {+ Assert(curBlock == BufferGetBlockNumber(victim_buffers[i]));+ Assert(BlockNumberIsValid(curBlock));++ /* don't release the pin on the page returned by this function */+ if (i > 0)+ ReleaseBuffer(victim_buffers[i]);++ if (i >= no_fsm_pages && use_fsm)+ {+ if (firstBlockFSM == InvalidBlockNumber)+ firstBlockFSM = curBlock;++ RecordPageWithFreeSpace(relation,+ curBlock,+ BufferGetPageSize(victim_buffers[i]) - SizeOfPageHeaderData);+ }+ }++ if (use_fsm && firstBlockFSM != InvalidBlockNumber)+ FreeSpaceMapVacuumRange(relation, firstBlockFSM, firstBlock + num_pages);++ if (bistate)+ {+ if (extend_by_pages > 1)+ {+ bistate->next_free = firstBlock + 1;+ bistate->last_free = firstBlock + extend_by_pages - 1;+ }+ else+ {+ bistate->next_free = InvalidBlockNumber;+ bistate->last_free = InvalidBlockNumber;+ }+ }++ buffer = victim_buffers[0];+ if (bistate)+ {+ IncrBufferRefCount(buffer);+ bistate->current_buf = buffer;+ }+#if 0+ ereport(LOG, errmsg("block start %u, size %zu, requested pages: %u, extend_by_pages: %d, waitcount: %d",+ firstBlock, len, num_pages, extend_by_pages, waitcount),+ errhidestmt(true), errhidecontext(true));+#endif+ }+#else
/*
* If we need the lock but are not able to acquire it immediately, we'll
* consider extending the relation by multiple blocks at a time to manage
@@ -635,6 +793,7 @@ loop:
*/
if (needLock)
UnlockRelationForExtension(relation, ExclusiveLock);
+#endif
/*
* We need to initialize the empty new page. Double-check that it really
--
2.38.0
[text/x-diff] v1-0012-bufmgr-debug-Add-PrintBuffer-Desc.patch (3.0K, ../20221029025420.eplyow6k7tgu6he3@awork3.anarazel.de/13-v1-0012-bufmgr-debug-Add-PrintBuffer-Desc.patch)
download | inline diff:
From be82f60f1a8097d042674678e177e6c28a4ebdfe Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Sun, 23 Oct 2022 14:41:46 -0700
Subject: [PATCH v1 12/12] bufmgr: debug: Add PrintBuffer[Desc]
Useful for development. Perhaps we should polish these and keep them?
---
src/include/storage/buf_internals.h | 3 ++
src/backend/storage/buffer/bufmgr.c | 49 +++++++++++++++++++++++++++++
2 files changed, 52 insertions(+)
diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.hindex 34b28eea906..337e94e4b4e 100644--- a/src/include/storage/buf_internals.h+++ b/src/include/storage/buf_internals.h@@ -390,6 +390,9 @@ extern void WritebackContextInit(WritebackContext *context, int *max_pending);
extern void IssuePendingWritebacks(WritebackContext *context);
extern void ScheduleBufferTagForWriteback(WritebackContext *context, BufferTag *tag);
+extern void PrintBuffer(Buffer buffer, const char *msg);+extern void PrintBufferDesc(BufferDesc *buf_hdr, const char *msg);+
/* freelist.c */
extern BufferDesc *StrategyGetBuffer(BufferAccessStrategy strategy,
uint32 *buf_state);
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.cindex 196b7d1ff25..a77a1205cf8 100644--- a/src/backend/storage/buffer/bufmgr.c+++ b/src/backend/storage/buffer/bufmgr.c@@ -3723,6 +3723,55 @@ DropDatabaseBuffers(Oid dbid)
* use only.
* -----------------------------------------------------------------
*/
++#include "utils/memutils.h"++void+PrintBufferDesc(BufferDesc *buf_hdr, const char *msg)+{+ Buffer buffer = BufferDescriptorGetBuffer(buf_hdr);+ uint32 buf_state = pg_atomic_read_u32(&buf_hdr->state);+ char *path = "";+ BlockNumber blockno = InvalidBlockNumber;++ CurrentMemoryContext->allowInCritSection = true;+ if (buf_state & BM_TAG_VALID)+ {+ path = relpathbackend(BufTagGetRelFileLocator(&buf_hdr->tag),+ InvalidBackendId, BufTagGetForkNum(&buf_hdr->tag));+ blockno = buf_hdr->tag.blockNum;+ }++ fprintf(stderr, "%d: [%u] msg: %s, rel: %s, block %u: refcount: %u / %u, usagecount: %u, flags:%s%s%s%s%s%s%s%s%s%s\n",+ MyProcPid,+ buffer,+ msg,+ path,+ blockno,+ BUF_STATE_GET_REFCOUNT(buf_state),+ GetPrivateRefCount(buffer),+ BUF_STATE_GET_USAGECOUNT(buf_state),+ buf_state & BM_LOCKED ? " BM_LOCKED" : "",+ buf_state & BM_DIRTY ? " BM_DIRTY" : "",+ buf_state & BM_VALID ? " BM_VALID" : "",+ buf_state & BM_TAG_VALID ? " BM_TAG_VALID" : "",+ buf_state & BM_IO_IN_PROGRESS ? " BM_IO_IN_PROGRESS" : "",+ buf_state & BM_IO_ERROR ? " BM_IO_ERROR" : "",+ buf_state & BM_JUST_DIRTIED ? " BM_JUST_DIRTIED" : "",+ buf_state & BM_PIN_COUNT_WAITER ? " BM_PIN_COUNT_WAITER" : "",+ buf_state & BM_CHECKPOINT_NEEDED ? " BM_CHECKPOINT_NEEDED" : "",+ buf_state & BM_PERMANENT ? " BM_PERMANENT" : ""+ );+}++void+PrintBuffer(Buffer buffer, const char *msg)+{+ BufferDesc *buf_hdr = GetBufferDescriptor(buffer - 1);++ PrintBufferDesc(buf_hdr, msg);+}+
#ifdef NOT_USED
void
PrintBufferDescs(void)
--
2.38.0
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: andres@anarazel.de, thomas.munro@gmail.com, melanieplageman@gmail.com, y.sokolov@postgrespro.ru, robertmhaas@gmail.com
Subject: Re: refactoring relation extension and BufferAlloc(), faster COPY
In-Reply-To: <20221029025420.eplyow6k7tgu6he3@awork3.anarazel.de>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox