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