Re: should frontend tools use syncfs() ?

On Wed, Aug 16, 2023 at 08:17:05AM -0700, Nathan Bossart wrote:
> On Wed, Aug 16, 2023 at 08:10:10AM +0900, Michael Paquier wrote:
>> On Tue, Aug 08, 2023 at 01:06:06PM -0700, Nathan Bossart wrote:
>> +        else
>> +        {
>> +            while (errno = 0, (de = readdir(dir)) != NULL)
>> +            {
>> +                char        subpath[MAXPGPATH * 2];
>> +
>> +                if (strcmp(de->d_name, ".") == 0 ||
>> +                    strcmp(de->d_name, "..") == 0)
>> +                    continue;
>> 
>> It seems to me that there is no need to do that for in-place
>> tablespaces.  There are relative paths in pg_tblspc/, so they would be
>> taken care of by the syncfs() done on the main data folder.
>> 
>> This does not really check if the mount points of each tablespace is
>> different, as well.  For example, imagine that you have two
>> tablespaces within the same disk, syncfs() twice.  Perhaps, the
>> current logic is OK anyway as long as the behavior is optional, but it
>> should be explained in the docs, at least.
> 
> True.  But I don't know if checking the mount point of each tablespace is
> worth the complexity.

Perhaps worth a note, this would depend on statvfs(), which is not
that portable the last time I looked at it (NetBSD, some BSD-ish?  And
of course WIN32).

> In the worst case, we'll call syncfs() on the same
> file system a few times, which is probably still much faster in most cases.
> FWIW this is what recovery_init_sync_method does today, and I'm not aware
> of any complaints about this behavior.

Hmm.  Okay.

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

>> I'm finding a bit confusing that fsync_pgdata() is coded in such a way
>> that it does a silent fallback to the cascading syncs through
>> walkdir() when syncfs is specified but not available in the build.
>> Perhaps an error is more helpful because one would then know that they
>> are trying something that's not around?
> 
> 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?

SyncMethod may be a bit too generic as name for the option structure.
How about a PGSyncMethod or pg_sync_method?

>> I am a bit concerned about the amount of duplication this patch
>> introduces in the docs.  Perhaps this had better be moved into a new
>> section of the docs to explain the tradeoffs, with each tool linking
>> to it?
> 
> 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).

>> Do we actually need --no-sync at all if --sync-method is around?  We
>> could have an extra --sync-method=none at option level with --no-sync
>> still around mainly for compatibility?  Or perhaps that's just
>> over-designing things?
> 
> I don't have a strong opinion.  We could take up deprecating --no-sync in a
> follow-up thread, though.  Like you said, we'll probably need to keep it
> around for backward compatibility, so it might not be worth the trouble.

Okay, maybe that's not worth it.
--
Michael