Avoid redundand memory allocation and sycnhronization in walredo #144

knizhnik · 2022-03-14T15:41:35Z

No description provided.

src/backend/storage/buffer/bufmgr.c

src/backend/tcop/zenith_wal_redo.c

hlinnaka · 2022-04-07T12:02:01Z

src/backend/tcop/zenith_wal_redo.c

+	MemoryContextSwitchTo(MessageContext);
+	initStringInfo(&input_message);
+


This makes us vulnerable to memory leaks. I guess that's OK; redo functions should not leak memory, or that would be a problem with normal WAL recovery too. We just have to be careful to not leak memory in our code in this file.

Unfortunately there is no fast way to get memory context size, otherwise I can add assert that size of MessageContext is not exceeding some limit.

src/backend/tcop/zenith_wal_redo.c

src/backend/storage/buffer/bufmgr.c

hlinnaka · 2022-04-12T17:49:46Z

contrib/zenith/inmem_smgr.c

@@ -41,6 +41,9 @@ inmem_init(void)
 {
 	HASHCTL		hashCtl;

+	if (inmem_files && hash_get_num_entries(inmem_files) == 0)


Does this make any difference? Every WAL redo call dirties a page, so 'inmem_files' will never be empty.

We could do one of the following:

make the hash table smaller; 1024 is much larger than is needed for any WAL record,

switch to simplehash. It's faster and has a reset-function,

switch to a simple array instead of a hash table. Almost all WAL records touch 1-4 pages, and linear scanning a simple array is faster than a hash table with such small numbers.

Does this make any difference? Every WAL redo call dirties a page, so 'inmem_files' will never be empty.

Oh, I think I get it now. These functions are only used for pages that are evicted from the buffer cache. That should pretty much never happen with a WAL redo function. Replaying a record won't affect more than a handful of pages. XLR_MAX_BLOCK_ID is 32, so not more than that, actually. Are inmem_write() and inmem_read() dead code in practice?

In any case, let's add comments about that!

And let's do this:

diff --git a/src/backend/tcop/zenith_wal_redo.c b/src/backend/tcop/zenith_wal_redo.c index 724c4d56bf..3cf4f0ef97 100644 --- a/src/backend/tcop/zenith_wal_redo.c +++ b/src/backend/tcop/zenith_wal_redo.c @@ -176,6 +176,8 @@ WalRedoMain(int argc, char *argv[], */ InitializeGUCOptions(); + num_temp_buffers = 64; + /* * Parse command-line options. * TODO

The default num_temp_buffers is 1024, which is much more than we need. And DropRelFileNodeAllLocalBuffers() scans through all buffers. In my quick profiling with 'perf', DropRelFileNodeAllLocalBuffers() was taking about 10% of the CPU time (of the WAL redo process), and after changing it to 64, it's not visible in the profile anymore.

Does this make any difference? Every WAL redo call dirties a page, so 'inmem_files' will never be empty.

Oh, I think I get it now. These functions are only used for pages that are evicted from the buffer cache. That should pretty much never happen with a WAL redo function. Replaying a record won't affect more than a handful of pages. XLR_MAX_BLOCK_ID is 32, so not more than that, actually. Are inmem_write() and inmem_read() dead code in practice?

In any case, let's add comments about that!

As far as I remember there are some cases when wal redo writes data directly to file, bypassing shared buffers. This is why this inmem_file is needed. Without it (without resetting files) some tests didn't pass.

We could do one of the following:

Actually even 32 (XLR_MAX_BLOCK_ID) pages are not needed because during recovery we restore only target page and ignoring all other.

I will try to check once again what is the maximal number of pages dirtied during wal redo (when it can be larger than 1).

And let's do this:

Makes sense: will do.

hlinnaka · 2022-04-12T17:55:11Z

src/backend/tcop/zenith_wal_redo.c

@@ -313,24 +318,23 @@ WalRedoMain(int argc, char *argv[],
 	/*
 	 * Main processing loop
 	 */
+	MemoryContextSwitchTo(MessageContext);
+	initStringInfo(&input_message);
+
 	for (;;)
 	{
 		/*
 		 * Release storage left over from prior query cycle, and create a new
 		 * query input buffer in the cleared MessageContext.


"cleared MessageContext" isn't accurate anymore, as we don't reset in the loop anymore.

Will fix this comment

knizhnik · 2022-04-19T14:47:24Z

I removed hash from inmem_smgr, reduce maximal size of pages to 4 and use sequential search for it.
Why 4? Just because 2 is not enough. Some XLOG records updating multiple forks requires more than 2 pages.
With 4 pages test_pg_regress is passed.

I also reduced number of temporary buffers to 128. 64 is not enough: hash initialization requires more buffers.

So I do not want to say that this limits (4 for inmem_smgr and 128 for num_temp_buffers) are absolutely safe.
I just checked that regression tests are passed with them.
We can leave simple hash implementation for inmem_smgr.

…redo postgres

Add comments on 'inmem_smgr.c', remove superfluous copy-pasted comments, pgindent.

hlinnaka · 2022-04-22T08:10:38Z

Added a commit with some minor cleanup here. Please squash before pushing

This brings us the performance improvements to WAL redo from neondatabase/postgres#144

* Avoid redundand memory allocation and sycnhronization in walredo * Address review comments * Reduce number of temp buffers and size of inmem file storage for wal redo postgres * Misc cleanup Add comments on 'inmem_smgr.c', remove superfluous copy-pasted comments, pgindent. Co-authored-by: Heikki Linnakangas <heikki.linnakangas@iki.fi>

antons-antons reviewed Mar 18, 2022

View reviewed changes

src/backend/storage/buffer/bufmgr.c Show resolved Hide resolved

Avoid redundand memory allocation and sycnhronization in walredo

f99d965

knizhnik force-pushed the wal_redo_optimization branch from 6171915 to f99d965 Compare April 7, 2022 11:40

knizhnik mentioned this pull request Apr 7, 2022

Optimize WAL redo speed neondatabase/neon#1362

Closed

hlinnaka approved these changes Apr 7, 2022

View reviewed changes

Address review comments

d6248f2

hlinnaka reviewed Apr 12, 2022

View reviewed changes

Reduce number of temp buffers and size of inmem file storage for wal …

2e33896

…redo postgres

knizhnik requested a review from hlinnaka April 22, 2022 07:14

Misc cleanup

78e77a8

Add comments on 'inmem_smgr.c', remove superfluous copy-pasted comments, pgindent.

hlinnaka approved these changes Apr 22, 2022

View reviewed changes

knizhnik merged commit 17213ed into main Apr 23, 2022

knizhnik deleted the wal_redo_optimization branch April 23, 2022 05:52

hlinnaka mentioned this pull request Apr 25, 2022

Bump vendor/postgres neondatabase/neon#1573

Merged

hlinnaka added a commit to neondatabase/neon that referenced this pull request Apr 28, 2022

Bump vendor/postgres

81b5c6b

This brings us the performance improvements to WAL redo from neondatabase/postgres#144

knizhnik pushed a commit to neondatabase/neon that referenced this pull request May 3, 2022

Bump vendor/postgres

7223ed6

This brings us the performance improvements to WAL redo from neondatabase/postgres#144

knizhnik pushed a commit to neondatabase/neon that referenced this pull request May 3, 2022

Bump vendor/postgres (#1573)

62449d6

This brings us the performance improvements to WAL redo from neondatabase/postgres#144

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid redundand memory allocation and sycnhronization in walredo #144

Avoid redundand memory allocation and sycnhronization in walredo #144

knizhnik commented Mar 14, 2022

hlinnaka Apr 7, 2022

knizhnik Apr 7, 2022

hlinnaka Apr 12, 2022

hlinnaka Apr 12, 2022

hlinnaka Apr 12, 2022

hlinnaka Apr 12, 2022

knizhnik Apr 13, 2022

knizhnik Apr 13, 2022

knizhnik Apr 13, 2022

hlinnaka Apr 12, 2022

knizhnik Apr 13, 2022

knizhnik commented Apr 19, 2022

hlinnaka commented Apr 22, 2022

		MemoryContextSwitchTo(MessageContext);
		initStringInfo(&input_message);

Avoid redundand memory allocation and sycnhronization in walredo #144

Avoid redundand memory allocation and sycnhronization in walredo #144

Conversation

knizhnik commented Mar 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knizhnik commented Apr 19, 2022

hlinnaka commented Apr 22, 2022