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+4-4
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.