On Fri, Sep 30, 2022 at 6:27 AM Nathan Bossart <nathandboss...@gmail.com> wrote: > On Thu, Sep 29, 2022 at 11:32:32AM +0530, Bharath Rupireddy wrote: > > On Sat, Sep 24, 2022 at 1:54 AM Nathan Bossart <nathandboss...@gmail.com> > > wrote: > >> > >> + PGAlignedXLogBlock zbuffer; > >> + > >> + memset(zbuffer.data, 0, XLOG_BLCKSZ); > >> > >> This seems excessive for only writing a single byte. > > > > Yes, I removed it now, instead doing pg_pwrite(fd, "\0", 1, > > wal_segment_size - 1). > > I don't think removing the use of PGAlignedXLogBlock here introduces any > sort of alignment risk, so this should be alright. > > +#ifdef WIN32 > + /* > + * WriteFile() on Windows changes the current file position, hence we > + * need an explicit lseek() here. See pg_pwrite() implementation in > + * win32pwrite.c for more details. > + */ > > Should we really surround this with a WIN32 check, or should we just > unconditionally lseek() here? I understand that this helps avoid an extra > system call on many platforms, but in theory another platform introduced in > the future could have the same problem, and this seems like something that > could easily be missed. Presumably we could do something fancier to > indicate pg_pwrite()'s behavior in this regard, but I don't know if that > sort of complexity is really worth it in order to save an lseek().
+1 for just doing it always, with a one-liner comment like "pg_pwritev*() might move the file position". No reason to spam the source tree with more explanations of the exact reason. If someone ever comes up with another case where p- and non-p- I/O functions are intermixed and it's really worth saving a system call (don't get me wrong, we call lseek() an obscene amount elsewhere and I'd like to fix that, but this case isn't hot?) then I like your idea of a macro to tell you whether you need to. Earlier I wondered why we'd want to include "pg_pwritev" in the name of this zero-filling function (pwritev being an internal implementation detail), but now it seems like maybe a good idea because it highlights the file position portability problem by being a member of that family of similarly-named functions.
