Re: should frontend tools use syncfs() ?

On Thu, Aug 17, 2023 at 12:50:31PM +0900, Michael Paquier wrote:
> On Wed, Aug 16, 2023 at 08:17:05AM -0700, Nathan Bossart wrote:
>> The patch does have the following note:
>> 
>> +        On Linux, <literal>syncfs</literal> may be used instead to ask the
>> +        operating system to synchronize the whole file systems that contain the
>> +        data directory, the WAL files, and each tablespace.
>> 
>> Do you think that is sufficient, or do you think we should really clearly
>> explain that you could end up calling syncfs() on the same file system a
>> few times if your tablespaces are on the same disk?  I personally feel
>> like that'd be a bit too verbose for the already lengthy descriptions of
>> this setting.
> 
> It does not hurt to mention that the code syncfs()-es each tablespace
> path (not in-place tablespaces), ignoring locations that share the
> same mounting point, IMO.  For that, we'd better rely on
> get_dirent_type() like the normal sync path.

But it doesn't ignore tablespace locations that share the same mount point.
It simply calls syncfs() for each tablespace path, just like
recovery_init_sync_method.

>> If syncfs() is not available, SYNC_METHOD_SYNCFS won't even be defined, and
>> parse_sync_method() should fail if "syncfs" is specified.  Furthermore, the
>> relevant part of fsync_pgdata() won't be compiled in whenever HAVE_SYNCFS
>> is not defined.
> 
> That feels structurally inconsistent with what we do with other
> option sets that have library dependencies.  For example, look at
> compression.h and what happens for pg_compress_algorithm.  So, it
> seems to me that it would be more friendly to list SYNC_METHOD_SYNCFS
> all the time in SyncMethod even if HAVE_SYNCFS is not around, and at
> least generate a warning rather than having a platform-dependent set
> of options?

Done.

> 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.

>> 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.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com