Re: should frontend tools use syncfs() ?

On Fri, Aug 18, 2023 at 09:01:11AM -0700, Nathan Bossart wrote:
> On Thu, Aug 17, 2023 at 12:50:31PM +0900, Michael Paquier wrote:
>> SyncMethod may be a bit too generic as name for the option structure.
>> How about a PGSyncMethod or pg_sync_method?
>
> In v4, I renamed this to DataDirSyncMethod and merged it with
> RecoveryInitSyncMethod. I'm not wedded to the name, but that seemed
> generic enough for both use-cases. As an aside, we need to be careful to
> distinguish these options from those for wal_sync_method.

Okay.

>>> Yeah, this crossed my mind. Do you know of any existing examples of
>>> options with links to a common section? One problem with this approach is
>>> that there are small differences in the wording for some of the frontend
>>> utilities, so it might be difficult to cleanly unite these sections.
>>
>> The closest thing I can think of is Color Support in section
>> Appendixes, that describes something shared across a lot of binaries
>> (that would be 6 tools with this patch).
>
> If I added a "syncfs() Caveats" appendix for the common parts of the docs,
> it would only say something like the following:
>
> Using syncfs may be a lot faster than using fsync, because it doesn't
> need to open each file one by one. On the other hand, it may be slower
> if a file system is shared by other applications that modify a lot of
> files, since those files will also be written to disk. Furthermore, on
> versions of Linux before 5.8, I/O errors encountered while writing data
> to disk may not be reported to the calling program, and relevant error
> messages may appear only in kernel logs.
>
> Does that seem reasonable? It would reduce the duplication a little bit,
> but I'm not sure it's really much of an improvement in this case.

This would cut 60% (?) of the documentation added by the patch for
these six tools, so that looks like an improvement to me. Perhaps
other may disagree, so more opinions are welcome.

--- a/src/include/storage/fd.h
+++ b/src/include/storage/fd.h
@@ -43,15 +43,11 @@
#ifndef FD_H
#define FD_H

+#ifndef FRONTEND
+
#include <dirent.h>
#include <fcntl.h>

Ugh. So you need this part because pg_rewind's filemap.c includes
fd.h, and pg_rewind also needs file_utils.h. This is not the fault of
your patch, but this does not make the situation better, either.. It
looks like we need to think harder about this layer. An improvement
would be to split file_utils.c so as its frontend-only code is moved
to OBJS_FRONTEND in a new file with a new header? It should be OK to
keep DataDirSyncMethod in file_utils.h as long as the split is clean.
--
Michael