Do not allocate shared memory for wal_redo process

[knizhnik]

No description provided.

[kelvich]

• Would be good to have a not null check
• Not completely sure, but seems that PGSharedMemoryDetach() will try to try to shmdt() returned address with the current approach.

I did a somewhat similar thing while playing with WASM, but one level up https://gist.github.com/kelvich/d0afaf4192574a3fc196263d441e6c04

I think a more clear approach would be to implement PGSharedMemoryCreate/PGSharedMemoryIsInUse/PGSharedMemoryDetach in a separate file like local_shmem.c and use it instead. However, that would be a build-time option so not completely sure.

[knizhnik]

shmdt is any case not enabled in secomp mode:

		/* Enable those for a proper shutdown.
		PG_SCMP_ALLOW(munmap),
		PG_SCMP_ALLOW(shmctl),
		PG_SCMP_ALLOW(shmdt),
		PG_SCMP_ALLOW(unlink), // shm_unlink
		*/

IMHO providing special implementation of PG shared memory (local_shmem.c) just for wal redo is overkill. Also it may be not so convenient to build two different versions of postgres binaries.

It is also possible to add pecial type for dynamic_shared_memory_type (right now sysv and mmap) and look at it. But it is also not so correct, because it referes only dynamic shared memory segments, not static.

From my point of view adding two lines of Neon specific code is better than other alternatives requiring more invasive changes of postgres core.

[hlinnaka]

Had to look up what valloc is, never heard of that before :-).

Why not malloc?

[hlinnaka]

Please add a comment

[knizhnik]

valloc returns address aligned on page boundary. It may affect alignment of some Postgres data structures. Not sure that it is critical for wal redo. But I thought that it will be better to preserve alignement.

[hlinnaka]

I agree with Stas' earlier comment that this needs a NULL check. If the valloc() call fails and returns NULL, I think this will retry the valloc() call in an infinite loop.

I also think we should replace the valloc() with plain malloc(). There is no reason for the allocation to be page-aligned.

BTW, with the default settings, this is only used to replace the tiny shmem shim allocation. Most of the shared memory is still allocated with the mmap() call. I think that's OK, as that codepath also does some tricks to make huge pages effective, which probably makes sense for the WAL redo process, too.

[knizhnik]

I agree with Stas' earlier comment that this needs a NULL check. If the valloc() call fails and returns NULL, I think this will retry the valloc() call in an infinite loop.

I also think we should replace the valloc() with plain malloc(). There is no reason for the allocation to be page-aligned.

BTW, with the default settings, this is only used to replace the tiny shmem shim allocation. Most of the shared memory is still allocated with the mmap() call. I think that's OK, as that codepath also does some tricks to make huge pages effective, which probably makes sense for the WAL redo process, too.

#173

I do not think that we should eliminate mmaps, because even malloc is using it internally.