Re: Use pg_pwritev_with_retry() instead of write() in dir_open_for_write() to avoid partial writes?

On Fri, Sep 30, 2022 at 6:27 AM Nathan Bossart <nathandbossart@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 <nathandbossart@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.