On Wed, 30 Jun 2021 at 06:53, Takashi Menjo <takashi.menjo@gmail.com> wrote:
>
> Rebased.
Thanks for these patches!
I recently took a dive into the WAL subsystem, and got to this
patchset through the previous ones linked from [0]. This patchset
seems straightforward to understand, so thanks!
I've looked over the patches and added some comments below. I haven't
yet tested this though; finding out how to get PMEM on WSL without
actual PMEM is probably going to be difficult.
> [ v3-0002-Add-wal_pmem_map-to-GUC.patch ]
> +extern bool wal_pmem_map;
A lot of the new code in these patches is gated behind this one flag,
but the flag should never be true on !pmem systems. Could you instead
replace it with something like the following?
+#ifdef USE_LIBPMEM
+extern bool wal_pmem_map;
+#else
+#define wal_pmem_map false
+#endif
A good compiler would then eliminate all the dead code from being
generated on non-pmem builds (instead of the compiler needing to keep
that code around just in case some extension decides to set
wal_pmem_map to true on !pmem systems because it has access to that
variable).
> [ v3-0004-Map-WAL-segment-files-on-PMEM-as-WAL-buffers.patch ]
> + if ((uintptr_t) addr & ~PG_DAX_HUGEPAGE_MASK)
> + elog(WARNING,
> + "file not mapped on DAX hugepage boundary: path \"%s\" addr %p",
> + path, addr);
I'm not sure that we should want to log this every time we detect the
issue; It's likely that once it happens it will happen for the next
file as well. Maybe add a timeout, or do we generally not deduplicate
such messages?
Kind regards,
Matthias
[0] https://wiki.postgresql.org/wiki/Persistent_Memory_for_WAL#Basic_performance
