CREATE DATABASE ... STRATEGY WAL_LOG issues
Hi,
While hacking on my relation extension patch I found two issues with WAL_LOG:
1) RelationCopyStorageUsingBuffer() doesn't free the used strategies. This
means we'll use #relations * ~10k memory
2) RelationCopyStorageUsingBuffer() gets the buffer for the target relation
with RBM_NORMAL, therefore requiring a read of a block guaranteed to be
zero
Easy enough to fix and shows clear improvement. One thing I wonder is if it's
worth moving the strategies up one level? Probaly not, but ...
Greetings,
Andres Freund
On Tue, Mar 21, 2023 at 3:01 AM Andres Freund <andres@anarazel.de> wrote:
While hacking on my relation extension patch I found two issues with WAL_LOG:
1) RelationCopyStorageUsingBuffer() doesn't free the used strategies. This
means we'll use #relations * ~10k memory
Woops.
2) RelationCopyStorageUsingBuffer() gets the buffer for the target relation
with RBM_NORMAL, therefore requiring a read of a block guaranteed to be
zero
Woops.
Easy enough to fix and shows clear improvement. One thing I wonder is if it's
worth moving the strategies up one level? Probaly not, but ...
Hmm, so share a strategy across all relation forks? You could even
push it up a level beyond that and share it across all relations being
copied. That feels like it would be slightly more rational behavior,
but I'm not smart enough to guess whether anyone would actually be
happier (or less happy) after such a change than they are now.
--
Robert Haas
EDB: http://www.enterprisedb.com
Hi,
On 2023-03-21 11:33:59 -0400, Robert Haas wrote:
On Tue, Mar 21, 2023 at 3:01 AM Andres Freund <andres@anarazel.de> wrote:
Easy enough to fix and shows clear improvement. One thing I wonder is if it's
worth moving the strategies up one level? Probaly not, but ...Hmm, so share a strategy across all relation forks? You could even
push it up a level beyond that and share it across all relations being
copied.
The latter is what I was wondering about.
That feels like it would be slightly more rational behavior,
but I'm not smart enough to guess whether anyone would actually be
happier (or less happy) after such a change than they are now.
Yea, I'm not either. The current behaviour does have the feature that it will
read in some data for each table, but limits trashing of shared buffers for
huge tables. That's good if your small to medium sized source database isn't
in s_b, because the next CREATE DATABASE has a change to not need to read the
data again. But if you have a source database with lots of small relations, it
can easily lead to swamping s_b.
More generally, I still think we need logic to use unused buffers even when
strategies are in use (my current theory is that we wouldn't increase the
usagecount when strategies use unused buffers, so they can be replaced more
easily).
Greetings,
Andres Freund
On Tue, Mar 21, 2023 at 12:34 PM Andres Freund <andres@anarazel.de> wrote:
More generally, I still think we need logic to use unused buffers even when
strategies are in use
Yep.
(my current theory is that we wouldn't increase the
usagecount when strategies use unused buffers, so they can be replaced more
easily).
Don't know about this part.
--
Robert Haas
EDB: http://www.enterprisedb.com
Hi,
On 2023-03-21 09:34:14 -0700, Andres Freund wrote:
On 2023-03-21 11:33:59 -0400, Robert Haas wrote:
That feels like it would be slightly more rational behavior,
but I'm not smart enough to guess whether anyone would actually be
happier (or less happy) after such a change than they are now.Yea, I'm not either. The current behaviour does have the feature that it will
read in some data for each table, but limits trashing of shared buffers for
huge tables. That's good if your small to medium sized source database isn't
in s_b, because the next CREATE DATABASE has a change to not need to read the
data again. But if you have a source database with lots of small relations, it
can easily lead to swamping s_b.
Patch with the two minimal fixes attached. As we don't know whether it's worth
changing the strategy, the more minimal fixes seem more appropriate.
Greetings,
Andres Freund
Attachments:
v1-0001-Fix-memory-leak-and-inefficiency-in-CREATE-DATABA.patchtext/x-diff; charset=us-asciiDownload
From caa172f4faeb4a1c2f8c03d9c31f15d91c97dde3 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Mon, 20 Mar 2023 21:57:40 -0700
Subject: [PATCH v1] Fix memory leak and inefficiency in CREATE DATABASE ...
STRATEGY WAL_LOG
RelationCopyStorageUsingBuffer() did not free the strategies used to access
the source / target relation. They memory was released at the end of the
transaction, but when using a template database with a lot of relations, the
temporary leak can become big prohibitively big.
RelationCopyStorageUsingBuffer() acquired the buffer for the target relation
with RBM_NORMAL, therefore requiring a read of a block guaranteed to be
zero. Use RBM_ZERO_AND_LOCK instead.
Discussion: https://postgr.es/m/20230321070113.o2vqqxogjykwgfrr@awork3.anarazel.de
---
src/backend/storage/buffer/bufmgr.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 0a05577b68d..95212a39416 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -3833,11 +3833,9 @@ RelationCopyStorageUsingBuffer(RelFileLocator srclocator,
LockBuffer(srcBuf, BUFFER_LOCK_SHARE);
srcPage = BufferGetPage(srcBuf);
- /* Use P_NEW to extend the destination relation. */
dstBuf = ReadBufferWithoutRelcache(dstlocator, forkNum, blkno,
- RBM_NORMAL, bstrategy_dst,
+ RBM_ZERO_AND_LOCK, bstrategy_dst,
permanent);
- LockBuffer(dstBuf, BUFFER_LOCK_EXCLUSIVE);
dstPage = BufferGetPage(dstBuf);
START_CRIT_SECTION();
@@ -3855,6 +3853,9 @@ RelationCopyStorageUsingBuffer(RelFileLocator srclocator,
UnlockReleaseBuffer(dstBuf);
UnlockReleaseBuffer(srcBuf);
}
+
+ FreeAccessStrategy(bstrategy_src);
+ FreeAccessStrategy(bstrategy_dst);
}
/* ---------------------------------------------------------------------
--
2.38.0
On Wed, Mar 22, 2023 at 1:12 AM Andres Freund <andres@anarazel.de> wrote:
Patch with the two minimal fixes attached. As we don't know whether it's worth
changing the strategy, the more minimal fixes seem more appropriate.
LGTM.
--
Robert Haas
EDB: http://www.enterprisedb.com
On 2023-03-22 09:58:58 -0400, Robert Haas wrote:
On Wed, Mar 22, 2023 at 1:12 AM Andres Freund <andres@anarazel.de> wrote:
Patch with the two minimal fixes attached. As we don't know whether it's worth
changing the strategy, the more minimal fixes seem more appropriate.LGTM.
Thanks for checking. Pushed.