Thread: should frontend tools use syncfs() ?
Forking this thread in which Thomas implemented syncfs for the startup process (61752afb2). https://www.postgresql.org/message-id/flat/CA%2BhUKG%2BSG9jSW3ekwib0cSdC0yD-jReJ21X4bZAmqxoWTLTc2A%40mail.gmail.com Is there any reason that initdb/pg_basebackup/pg_checksums/pg_rewind shouldn't use syncfs() ? do_syncfs() is in src/backend/ so would need to be duplicated^Wimplemented in common. They can't use the GUC, so need to add an cmdline option or look at an environment variable.
Attachment
On Wed, Sep 29, 2021 at 07:43:41PM -0500, Justin Pryzby wrote: > Forking this thread in which Thomas implemented syncfs for the startup process > (61752afb2). > https://www.postgresql.org/message-id/flat/CA%2BhUKG%2BSG9jSW3ekwib0cSdC0yD-jReJ21X4bZAmqxoWTLTc2A%40mail.gmail.com > > Is there any reason that initdb/pg_basebackup/pg_checksums/pg_rewind shouldn't > use syncfs() ? That makes sense. > do_syncfs() is in src/backend/ so would need to be duplicated^Wimplemented in > common. The fd handling in the backend makes things tricky if trying to plug in a common interface, so I'd rather do that as this is frontend-only code. > They can't use the GUC, so need to add an cmdline option or look at an > environment variable. fsync_pgdata() is going to manipulate many inodes anyway, because that's a code path designed to do so. If we know that syncfs() is just going to be better, I'd rather just call it by default if available and not add new switches to all the frontend tools in need of flushing the data folder, switches that are not documented in your patch. -- Michael
Attachment
On Thu, Sep 30, 2021 at 4:49 PM Michael Paquier <michael@paquier.xyz> wrote: > fsync_pgdata() is going to manipulate many inodes anyway, because > that's a code path designed to do so. If we know that syncfs() is > just going to be better, I'd rather just call it by default if > available and not add new switches to all the frontend tools in need > of flushing the data folder, switches that are not documented in your > patch. If we want this it should be an option, because it flushes out data other than the pgdata dir, and it doesn't report errors on old kernels.
On Thu, Sep 30, 2021 at 05:08:24PM +1300, Thomas Munro wrote:
> If we want this it should be an option, because it flushes out data
> other than the pgdata dir, and it doesn't report errors on old
> kernels.
Oh, OK, thanks. That's the part about 5.8. The only option
controlling if sync is used now in those binaries is --no-sync.
Should we use a different design for the option rather than a
--syncfs? Something like --sync={on,off,syncfs,fsync} could be a
possibility, for example.
--
Michael
Attachment
On Thu, Sep 30, 2021 at 05:08:24PM +1300, Thomas Munro wrote: > On Thu, Sep 30, 2021 at 4:49 PM Michael Paquier <michael@paquier.xyz> wrote: > > fsync_pgdata() is going to manipulate many inodes anyway, because > > that's a code path designed to do so. If we know that syncfs() is > > just going to be better, I'd rather just call it by default if > > available and not add new switches to all the frontend tools in need > > of flushing the data folder, switches that are not documented in your > > patch. > > If we want this it should be an option, because it flushes out data > other than the pgdata dir, and it doesn't report errors on old > kernels. I ran into bad performance of initdb --sync-only shortly after adding it to my db migration script, so added initdb --syncfs. I found that with sufficiently recent coreutils, I can do what's wanted by calling /bin/sync -f /datadir Since it's not integrated into initdb, it's necessary to include each tablespace and wal. -- Justin
On Thu, Sep 30, 2021 at 12:49:36PM +0900, Michael Paquier wrote: > On Wed, Sep 29, 2021 at 07:43:41PM -0500, Justin Pryzby wrote: > > Forking this thread in which Thomas implemented syncfs for the startup process > > (61752afb2). > > https://www.postgresql.org/message-id/flat/CA%2BhUKG%2BSG9jSW3ekwib0cSdC0yD-jReJ21X4bZAmqxoWTLTc2A%40mail.gmail.com > > > > Is there any reason that initdb/pg_basebackup/pg_checksums/pg_rewind shouldn't > > use syncfs() ? > > That makes sense. > > > do_syncfs() is in src/backend/ so would need to be duplicated^Wimplemented in > > common. > > The fd handling in the backend makes things tricky if trying to plug > in a common interface, so I'd rather do that as this is frontend-only > code. > > > They can't use the GUC, so need to add an cmdline option or look at an > > environment variable. > > fsync_pgdata() is going to manipulate many inodes anyway, because > that's a code path designed to do so. If we know that syncfs() is > just going to be better, I'd rather just call it by default if > available and not add new switches to all the frontend tools in need > of flushing the data folder, switches that are not documented in your > patch. It is a draft/POC, after all. The argument against using syncfs by default is that it could be worse than recursive fsync if a tiny 200MB postgres instance lives on a shared filesystem along with other, larger applications (maybe a larger postgres instance). There's also an argument that syncfs might be unreliable in the case of a write error. (But I agreed with Thomas' earlier assessment: that claim caries little weight since fsync() itself wasn't reliable for 20some years). I didn't pursue this patch, as it's easier for me to use /bin/sync -f. Someone should adopt it if interested. -- Justin
On Wed, Apr 13, 2022 at 06:54:12AM -0500, Justin Pryzby wrote: > I didn't pursue this patch, as it's easier for me to use /bin/sync -f. Someone > should adopt it if interested. I was about to start a new thread, but I found this one with some good preliminary discussion. I came to the same conclusion about introducing a new option instead of using syncfs() by default wherever it is available. The attached patch is still a work-in-progress, but it seems to behave as expected. I began investigating this because I noticed that the sync-data-directory step on pg_upgrade takes quite a while when there are many files, and I am looking for ways to reduce the amount of downtime required for pg_upgrade. The attached patch adds a new --sync-method option to the relevant frontend utilities, but I am not wedded to that name/approach. Thoughts? -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Sat, Jul 29, 2023 at 02:40:10PM -0700, Nathan Bossart wrote: > I was about to start a new thread, but I found this one with some good > preliminary discussion. I came to the same conclusion about introducing a > new option instead of using syncfs() by default wherever it is available. > The attached patch is still a work-in-progress, but it seems to behave as > expected. I began investigating this because I noticed that the > sync-data-directory step on pg_upgrade takes quite a while when there are > many files, and I am looking for ways to reduce the amount of downtime > required for pg_upgrade. > > The attached patch adds a new --sync-method option to the relevant frontend > utilities, but I am not wedded to that name/approach. Here is a new version of the patch with documentation updates and a couple other small improvements. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Mon, Jul 31, 2023 at 10:51:38AM -0700, Nathan Bossart wrote: > Here is a new version of the patch with documentation updates and a couple > other small improvements. I just realized I forgot to update the --help output for these utilities. I'll do that in the next version of the patch. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Mon, Jul 31, 2023 at 11:39:46AM -0700, Nathan Bossart wrote: > I just realized I forgot to update the --help output for these utilities. > I'll do that in the next version of the patch. Done in v3. Sorry for the noise. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
I ran a couple of tests for pg_upgrade with 100k tables (created using the
script here [0]) in order to demonstrate the potential benefits of this
patch.
pg_upgrade --sync-method fsync
real 5m50.072s
user 0m10.606s
sys 0m40.298s
pg_upgrade --sync-method syncfs
real 3m44.096s
user 0m8.906s
sys 0m26.398s
pg_upgrade --no-sync
real 3m27.697s
user 0m9.056s
sys 0m26.605s
[0] https://postgr.es/m/3612876.1689443232%40sss.pgh.pa.us
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Tue, Aug 08, 2023 at 01:06:06PM -0700, Nathan Bossart wrote:
> I ran a couple of tests for pg_upgrade with 100k tables (created using the
> script here [0]) in order to demonstrate the potential benefits of this
> patch.
That shows some nice numbers with many files, indeed. How does the
size of each file influence the difference in time?
+ 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.
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?
+ pg_log_error("could not synchronize file system for file \"%s\": %m", path);
+ (void) close(fd);
+ exit(EXIT_FAILURE);
walkdir() reports errors and does not consider these fatal. Why the
early exit()?
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?
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?
--
Michael
Attachment
Thanks for taking a look.
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:
>> I ran a couple of tests for pg_upgrade with 100k tables (created using the
>> script here [0]) in order to demonstrate the potential benefits of this
>> patch.
>
> That shows some nice numbers with many files, indeed. How does the
> size of each file influence the difference in time?
IME the number of files tends to influence the duration much more than the
size. I assume this is because most files are already sync'd in these code
paths that loop through every file.
> + 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. 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.
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.
> 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.
> + pg_log_error("could not synchronize file system for file \"%s\": %m", path);
> + (void) close(fd);
> + exit(EXIT_FAILURE);
>
> walkdir() reports errors and does not consider these fatal. Why the
> early exit()?
I know it claims to, but fsync_fname() does exit when fsync() fails:
returncode = fsync(fd);
/*
* Some OSes don't allow us to fsync directories at all, so we can ignore
* those errors. Anything else needs to be reported.
*/
if (returncode != 0 && !(isdir && (errno == EBADF || errno == EINVAL)))
{
pg_log_error("could not fsync file \"%s\": %m", fname);
(void) close(fd);
exit(EXIT_FAILURE);
}
I suspect that the current code does not treat failures for things like
open() as fatal because it's likely due to a lack of permissions on the
file, but it does treat failures to fsync() as fatal because it is more
likely to indicate that ѕomething is very wrong. I don't know whether this
reasoning is sound, but I tried to match the current convention in the
syncfs() code.
> 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.
> 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.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
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:
>> + pg_log_error("could not synchronize file system for file \"%s\": %m", path);
>> + (void) close(fd);
>> + exit(EXIT_FAILURE);
>>
>> walkdir() reports errors and does not consider these fatal. Why the
>> early exit()?
>
> I know it claims to, but fsync_fname() does exit when fsync() fails:
>
> returncode = fsync(fd);
>
> /*
> * Some OSes don't allow us to fsync directories at all, so we can ignore
> * those errors. Anything else needs to be reported.
> */
> if (returncode != 0 && !(isdir && (errno == EBADF || errno == EINVAL)))
> {
> pg_log_error("could not fsync file \"%s\": %m", fname);
> (void) close(fd);
> exit(EXIT_FAILURE);
> }
>
> I suspect that the current code does not treat failures for things like
> open() as fatal because it's likely due to a lack of permissions on the
> file, but it does treat failures to fsync() as fatal because it is more
> likely to indicate that ѕomething is very wrong. I don't know whether this
> reasoning is sound, but I tried to match the current convention in the
> syncfs() code.
Ah, it looks like this code used to treat fsync() errors as non-fatal, but
it was changed in commit 1420617. I still find it a bit strange that some
errors that prevent a file from being sync'd are non-fatal while others
_are_ fatal, but that is probably a topic for another thread.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Wed, Aug 16, 2023 at 08:23:25AM -0700, Nathan Bossart wrote: > Ah, it looks like this code used to treat fsync() errors as non-fatal, but > it was changed in commit 1420617. I still find it a bit strange that some > errors that prevent a file from being sync'd are non-fatal while others > _are_ fatal, but that is probably a topic for another thread. Right. That rings a bell. -- Michael
Attachment
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
Attachment
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
Attachment
On Wed, Aug 16, 2023 at 11:50 PM Michael Paquier <michael@paquier.xyz> wrote: > >> 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. Doesn't seem worth it to me. I think --no-sync is more intuitive than --sync-method=none, it's certainly shorter, and it's a pretty important setting because we use it when running the regression tests. -- Robert Haas EDB: http://www.enterprisedb.com
On Mon, Aug 21, 2023 at 04:08:46PM -0400, Robert Haas wrote: > Doesn't seem worth it to me. I think --no-sync is more intuitive than > --sync-method=none, it's certainly shorter, and it's a pretty > important setting because we use it when running the regression tests. No arguments against that ;) -- Michael
Attachment
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
Attachment
On Tue, Aug 22, 2023 at 08:56:26AM +0900, Michael Paquier wrote:
> --- 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.
I'm hoping there's a simpler path forward here. pg_rewind only needs the
following lines from fd.h:
/* Filename components */
#define PG_TEMP_FILES_DIR "pgsql_tmp"
#define PG_TEMP_FILE_PREFIX "pgsql_tmp"
Maybe we could move these to file_utils.h instead. WDYT?
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Mon, Aug 21, 2023 at 06:44:07PM -0700, Nathan Bossart wrote: > I'm hoping there's a simpler path forward here. pg_rewind only needs the > following lines from fd.h: > > /* Filename components */ > #define PG_TEMP_FILES_DIR "pgsql_tmp" > #define PG_TEMP_FILE_PREFIX "pgsql_tmp" > > Maybe we could move these to file_utils.h instead. WDYT? I guess so.. At the same time, something can be said about pg_checksums that redeclares PG_TEMP_FILE_PREFIX and PG_TEMP_FILES_DIR because it does not want to include fd.h and its sync routines. -- Michael
Attachment
On Tue, Aug 22, 2023 at 10:50:01AM +0900, Michael Paquier wrote: > On Mon, Aug 21, 2023 at 06:44:07PM -0700, Nathan Bossart wrote: >> I'm hoping there's a simpler path forward here. pg_rewind only needs the >> following lines from fd.h: >> >> /* Filename components */ >> #define PG_TEMP_FILES_DIR "pgsql_tmp" >> #define PG_TEMP_FILE_PREFIX "pgsql_tmp" >> >> Maybe we could move these to file_utils.h instead. WDYT? > > I guess so.. At the same time, something can be said about > pg_checksums that redeclares PG_TEMP_FILE_PREFIX and PG_TEMP_FILES_DIR > because it does not want to include fd.h and its sync routines. This would look something like the attached patch. I think this is nicer. With this patch, we don't have to choose between including fd.h or redefining the macros in the frontend code. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Mon, Aug 21, 2023 at 07:06:32PM -0700, Nathan Bossart wrote: > This would look something like the attached patch. I think this is nicer. > With this patch, we don't have to choose between including fd.h or > redefining the macros in the frontend code. Yes, this one is moving the needle in the good direction. +1. -- Michael
Attachment
On Tue, Aug 22, 2023 at 12:53:53PM +0900, Michael Paquier wrote: > On Mon, Aug 21, 2023 at 07:06:32PM -0700, Nathan Bossart wrote: >> This would look something like the attached patch. I think this is nicer. >> With this patch, we don't have to choose between including fd.h or >> redefining the macros in the frontend code. > > Yes, this one is moving the needle in the good direction. +1. Great. Here is a new patch set that includes this change as well as the suggested documentation updates. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
rebased -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Tue, Aug 29, 2023 at 08:45:59AM -0700, Nathan Bossart wrote: > rebased 0001 looks OK, worth its own, independent, commit. I understand that I'm perhaps sounding pedantic about fsync_pgdata().. But, after thinking more about it, I would still make this code fail hard with an exit(EXIT_FAILURE) to let any C code calling directly this routine with sync_method = DATA_DIR_SYNC_METHOD_SYNCFS know that the build does not allow the use of this option when we don't have HAVE_SYNCFS. parse_sync_method() offers some protection, but adding this restriction also in the execution path is more friendly than falling back silently to the default of flushing each file if fsync_pgdata() is called with syncfs but the build does not support it. At least that's more predictible. I'm fine with the doc changes. -- Michael
Attachment
On Wed, Aug 30, 2023 at 09:10:47AM +0900, Michael Paquier wrote: > I understand that I'm perhaps sounding pedantic about fsync_pgdata().. > But, after thinking more about it, I would still make this code fail > hard with an exit(EXIT_FAILURE) to let any C code calling directly > this routine with sync_method = DATA_DIR_SYNC_METHOD_SYNCFS know that > the build does not allow the use of this option when we don't have > HAVE_SYNCFS. parse_sync_method() offers some protection, but adding > this restriction also in the execution path is more friendly than > falling back silently to the default of flushing each file if > fsync_pgdata() is called with syncfs but the build does not support > it. At least that's more predictible. That seems fair enough. I did this in v7. I restructured fsync_pgdata() and fsync_dir_recurse() so that any new sync methods should cause compiler warnings until they are implemented. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Tue, Aug 29, 2023 at 06:14:08PM -0700, Nathan Bossart wrote: > That seems fair enough. I did this in v7. I restructured fsync_pgdata() > and fsync_dir_recurse() so that any new sync methods should cause compiler > warnings until they are implemented. That's pretty cool and easier to maintain in the long term. After sleeping on it, there are two things that popped up in my mind that may be worth considering: - Should we have some regression tests? We should only need one test in one of the binaries to be able to stress the new code paths of file_utils.c with syncfs. The cheapest one may be pg_dump with a dump in directory format? Note that we have tests there that depend on lz4 or gzip existing, which are conditional. - Perhaps 0002 should be split into two parts? The first patch could introduce DataDirSyncMethod in file_utils.h with the new routines in file_utils.h (including syncfs support), and the second patch would plug the new option to all the binaries. In the first patch, I would hardcode DATA_DIR_SYNC_METHOD_FSYNC. -- Michael
Attachment
On Thu, Aug 31, 2023 at 02:30:33PM +0900, Michael Paquier wrote: > - Should we have some regression tests? We should only need one test > in one of the binaries to be able to stress the new code paths of > file_utils.c with syncfs. The cheapest one may be pg_dump with a > dump in directory format? Note that we have tests there that depend > on lz4 or gzip existing, which are conditional. I added one for initdb in v8. > - Perhaps 0002 should be split into two parts? The first patch could > introduce DataDirSyncMethod in file_utils.h with the new routines in > file_utils.h (including syncfs support), and the second patch would > plug the new option to all the binaries. In the first patch, I would > hardcode DATA_DIR_SYNC_METHOD_FSYNC. Ha, I was just thinking about this, too. I actually split it into 3 patches. The first adds DataDirSyncMethod and uses it for recovery_init_sync_method. The second adds syncfs() support in file_utils.c. And the third adds the ability to specify syncfs in the frontend utilities. WDYT? -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Thu, Aug 31, 2023 at 08:48:58AM -0700, Nathan Bossart wrote:
> On Thu, Aug 31, 2023 at 02:30:33PM +0900, Michael Paquier wrote:
> > - Should we have some regression tests? We should only need one test
> > in one of the binaries to be able to stress the new code paths of
> > file_utils.c with syncfs. The cheapest one may be pg_dump with a
> > dump in directory format? Note that we have tests there that depend
> > on lz4 or gzip existing, which are conditional.
>
> I added one for initdb in v8.
+my $supports_syncfs = check_pg_config("#define HAVE_SYNCFS 1");
That should be OK this way. The extra running time is not really
visible, right?
+command_ok([ 'initdb', '-S', $datadir, '--sync-method', 'fsync' ],
+ 'sync method fsync');
Removing this one may be fine, actually, because we test the sync
paths on other places like pg_dump.
> Ha, I was just thinking about this, too. I actually split it into 3
> patches. The first adds DataDirSyncMethod and uses it for
> recovery_init_sync_method. The second adds syncfs() support in
> file_utils.c. And the third adds the ability to specify syncfs in the
> frontend utilities. WDYT?
This split is OK by me, so WFM.
--
Michael
Attachment
On Fri, Sep 01, 2023 at 10:40:12AM +0900, Michael Paquier wrote: > That should be OK this way. The extra running time is not really > visible, right? AFAICT it is negligible. Presumably it could take a little longer if there is a lot to sync on the file system, but I don't know if that's worth worrying about. > +command_ok([ 'initdb', '-S', $datadir, '--sync-method', 'fsync' ], > + 'sync method fsync'); > > Removing this one may be fine, actually, because we test the sync > paths on other places like pg_dump. Done. > This split is OK by me, so WFM. Cool. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
> + if (!user_opts.sync_method)
> + user_opts.sync_method = pg_strdup("fsync");
why pstrdup?
> +parse_sync_method(const char *optarg, SyncMethod *sync_method)
> +{
> + if (strcmp(optarg, "fsync") == 0)
> + *sync_method = SYNC_METHOD_FSYNC;
> +#ifdef HAVE_SYNCFS
> + else if (strcmp(optarg, "syncfs") == 0)
> + *sync_method = SYNC_METHOD_SYNCFS;
> +#endif
> + else
> + {
> + pg_log_error("unrecognized sync method: %s", optarg);
> + return false;
> + }
This should probably give a distinct error when syncfs is not supported
than when it's truely recognized.
The patch should handle pg_dumpall, too.
Note that /bin/sync doesn't try to de-duplicate, it does just what you
tell it.
$ strace -e openat,syncfs,fsync sync / / / -f
...
openat(AT_FDCWD, "/", O_RDONLY|O_NONBLOCK) = 3
syncfs(3) = 0
openat(AT_FDCWD, "/", O_RDONLY|O_NONBLOCK) = 3
syncfs(3) = 0
openat(AT_FDCWD, "/", O_RDONLY|O_NONBLOCK) = 3
syncfs(3) = 0
+++ exited with 0 +++
Thanks for taking a look.
On Fri, Sep 01, 2023 at 12:58:10PM -0500, Justin Pryzby wrote:
>> + if (!user_opts.sync_method)
>> + user_opts.sync_method = pg_strdup("fsync");
>
> why pstrdup?
I believe I was just following the precedent set by some of the other
options.
>> +parse_sync_method(const char *optarg, SyncMethod *sync_method)
>> +{
>> + if (strcmp(optarg, "fsync") == 0)
>> + *sync_method = SYNC_METHOD_FSYNC;
>> +#ifdef HAVE_SYNCFS
>> + else if (strcmp(optarg, "syncfs") == 0)
>> + *sync_method = SYNC_METHOD_SYNCFS;
>> +#endif
>> + else
>> + {
>> + pg_log_error("unrecognized sync method: %s", optarg);
>> + return false;
>> + }
>
> This should probably give a distinct error when syncfs is not supported
> than when it's truely recognized.
Later versions of the patch should have this.
> The patch should handle pg_dumpall, too.
It looks like pg_dumpall only ever fsyncs a single file, so I don't think
it is really needed there.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Fri, Sep 01, 2023 at 11:08:51AM -0700, Nathan Bossart wrote: > > This should probably give a distinct error when syncfs is not supported > > than when it's truely recognized. > > Later versions of the patch should have this. Oops, right. > > The patch should handle pg_dumpall, too. > > It looks like pg_dumpall only ever fsyncs a single file, so I don't think > it is really needed there. What about (per git grep no-sync doc) pg_receivewal? -- Justin
On Fri, Sep 01, 2023 at 01:19:13PM -0500, Justin Pryzby wrote: > What about (per git grep no-sync doc) pg_receivewal? I don't think it's applicable there, either. IIUC that option specifies whether to sync the data as it is streamed over. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
I've committed 0001 for now. I plan to commit the rest in the next couple of days. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Tue, Sep 05, 2023 at 05:08:53PM -0700, Nathan Bossart wrote: > I've committed 0001 for now. I plan to commit the rest in the next couple > of days. Committed. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Thu, 7 Sept 2023 at 03:34, Nathan Bossart <nathandbossart@gmail.com> wrote:
Committed.
Hi! Great job!
But here is one problem I've encountered during working on some unrelated stuff.
How we have two different things call the same name – sync_method. One in xlog:
int sync_method = DEFAULT_SYNC_METHOD;
...and another one in "bins":
static DataDirSyncMethod sync_method = DATA_DIR_SYNC_METHOD_FSYNC;
In current include order, this is not a problem, but imagine you add a couple of new includes,
for example:
--- a/src/include/storage/bufpage.h
+++ b/src/include/storage/bufpage.h
@@ -18,6 +18,8 @@
#include "storage/block.h"
#include "storage/item.h"
#include "storage/off.h"
+#include "postgres.h"
+#include "utils/rel.h"
+++ b/src/include/storage/bufpage.h
@@ -18,6 +18,8 @@
#include "storage/block.h"
#include "storage/item.h"
#include "storage/off.h"
+#include "postgres.h"
+#include "utils/rel.h"
And build will be broken, because we how have two different things called "sync_method" with
different types:
In file included from .../src/bin/pg_rewind/pg_rewind.c:33:
In file included from .../src/include/storage/bufpage.h:22:
In file included from .../src/include/utils/rel.h:18:
.../src/include/access/xlog.h:27:24: error: redeclaration of 'sync_method' with a different type: 'int' vs 'DataDirSyncMethod' (aka 'enum DataDirSyncMethod')
extern PGDLLIMPORT int sync_method;
...
In file included from .../src/include/storage/bufpage.h:22:
In file included from .../src/include/utils/rel.h:18:
.../src/include/access/xlog.h:27:24: error: redeclaration of 'sync_method' with a different type: 'int' vs 'DataDirSyncMethod' (aka 'enum DataDirSyncMethod')
extern PGDLLIMPORT int sync_method;
...
As a solution, I suggest renaming sync_method in xlog module to wal_sync_method. In fact,
appropriate GUC for this variable, called "wal_sync_method" and I see no reason not to use
the exact same name for a variable in xlog module.
--
Best regards,
Maxim Orlov.
Attachment
On Wed, Sep 20, 2023 at 03:12:56PM +0300, Maxim Orlov wrote: > As a solution, I suggest renaming sync_method in xlog module to > wal_sync_method. In fact, > appropriate GUC for this variable, called "wal_sync_method" and I see no > reason not to use > the exact same name for a variable in xlog module. +1 I think we should also consider renaming things like SYNC_METHOD_FSYNC to WAL_SYNC_METHOD_FSYNC, and sync_method_options to wal_sync_method_options. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Wed, 20 Sept 2023 at 22:08, Nathan Bossart <nathandbossart@gmail.com> wrote:
I think we should also consider renaming things like SYNC_METHOD_FSYNC to
WAL_SYNC_METHOD_FSYNC, and sync_method_options to wal_sync_method_options.
I've already rename sync_method_options in previous patch.
34 @@ -171,7 +171,7 @@ static bool check_wal_consistency_checking_deferred = false;
35 /*
36 * GUC support
37 */
38 -const struct config_enum_entry sync_method_options[] = {
39 +const struct config_enum_entry wal_sync_method_options[] = {
35 /*
36 * GUC support
37 */
38 -const struct config_enum_entry sync_method_options[] = {
39 +const struct config_enum_entry wal_sync_method_options[] = {
As for SYNC_METHOD_FSYNC rename, PFA patch.
Also make enum for WAL sync methods instead of defines.
--
Best regards,
Maxim Orlov.
Attachment
On 17.08.23 04:50, Michael Paquier wrote: >> 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). I think it's a bit much to add a whole appendix for that little content. We have a collection of platform-specific notes in chapter 19, including file-system-related notes in section 19.2. Maybe it could be put there?
On Wed, Sep 27, 2023 at 01:56:08PM +0100, Peter Eisentraut wrote: > I think it's a bit much to add a whole appendix for that little content. I'm inclined to agree. > We have a collection of platform-specific notes in chapter 19, including > file-system-related notes in section 19.2. Maybe it could be put there? I will give this a try. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Back to the patch v11. I don’t understand a bit, what we should do next? Make a separate thread or put this one on commitfest?
--
Best regards,
Maxim Orlov.
On Fri, Oct 06, 2023 at 10:50:11AM +0300, Maxim Orlov wrote: > Back to the patch v11. I don’t understand a bit, what we should do next? > Make a separate thread or put this one on commitfest? From a quick skim, this one looks pretty good to me. Would you mind adding it to the commitfest so that it doesn't get lost? I will aim to take a closer look at it next week. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Fri, 6 Oct 2023 at 22:35, Nathan Bossart <nathandbossart@gmail.com> wrote:
From a quick skim, this one looks pretty good to me. Would you mind adding
it to the commitfest so that it doesn't get lost? I will aim to take a
closer look at it next week.
Sounds good, thanks a lot!
--
Best regards,
Maxim Orlov.
On Mon, Oct 09, 2023 at 01:12:16PM +0300, Maxim Orlov wrote: > On Fri, 6 Oct 2023 at 22:35, Nathan Bossart <nathandbossart@gmail.com> > wrote: >> From a quick skim, this one looks pretty good to me. Would you mind adding >> it to the commitfest so that it doesn't get lost? I will aim to take a >> closer look at it next week. > > Sounds good, thanks a lot! > > https://commitfest.postgresql.org/45/4609/ Thanks. I've made a couple of small changes, but otherwise I think this one is just about ready. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Mon, Oct 09, 2023 at 11:14:39AM -0500, Nathan Bossart wrote: > Thanks. I've made a couple of small changes, but otherwise I think this > one is just about ready. I forgot to rename one thing. Here's a v13 with that fixed. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Wed, Oct 04, 2023 at 10:29:07AM -0500, Nathan Bossart wrote: > On Wed, Sep 27, 2023 at 01:56:08PM +0100, Peter Eisentraut wrote: >> We have a collection of platform-specific notes in chapter 19, including >> file-system-related notes in section 19.2. Maybe it could be put there? > > I will give this a try. I started on this, but I couldn't shake the feeling that this wasn't the right place for these notes. This chapter is about setting up a server, and the syncfs() notes do apply to the recovery_init_sync_method configuration parameter, but it also applies to a number of server/client applications. I've been looking around, and I haven't found a great place to move this section to. IMO some of the other appendices have similar amounts of information (e.g., Date/Time Support, The Source Code Repository, Color Support), so maybe a dedicated appendix isn't too extreme. Another option could be to introduce a new section for platform-specific notes, but that would just make this section even larger for now. Thoughts? -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Mon, Oct 09, 2023 at 02:34:27PM -0500, Nathan Bossart wrote: > On Mon, Oct 09, 2023 at 11:14:39AM -0500, Nathan Bossart wrote: >> Thanks. I've made a couple of small changes, but otherwise I think this >> one is just about ready. > > I forgot to rename one thing. Here's a v13 with that fixed. Committed. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com