On Wed, 30 Jun 2021 at 06:53, Takashi Menjo <takashi.me...@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
