Обсуждение: fsync error handling in pg_receivewal, pg_recvlogical

Do we need to review the fsync error handling in pg_receivewal and
pg_recvlogical, following the recent backend changes?  The current
default behavior is that these tools will log fsync errors and then
reconnect and proceed with the next data streaming in.  As a result, you
might then have some files in the accumulated WAL that have not been
fsynced.  Perhaps a hard exit would be more appropriate?

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Mar 29, 2019 at 12:48:09PM +0100, Peter Eisentraut wrote:
> Do we need to review the fsync error handling in pg_receivewal and
> pg_recvlogical, following the recent backend changes?  The current
> default behavior is that these tools will log fsync errors and then
> reconnect and proceed with the next data streaming in.  As a result, you
> might then have some files in the accumulated WAL that have not been
> fsynced.  Perhaps a hard exit would be more appropriate?

Yes, I think that we are going to need an equivalent of that for all
frontend tools.  At various degrees, making sure that a fsync happens
is also important for pg_dump, pg_basebackup, pg_rewind and
pg_checksums so it is not only a problem of the two tools you mention.
It seems to me that the most correct way to have those failures would
be to use directly exit(EXIT_FAILURE) in file_utils.c where
appropriate.
--
Michael

On 2019-03-29 14:05, Michael Paquier wrote:
> Yes, I think that we are going to need an equivalent of that for all
> frontend tools.  At various degrees, making sure that a fsync happens
> is also important for pg_dump, pg_basebackup, pg_rewind and
> pg_checksums so it is not only a problem of the two tools you mention.
> It seems to me that the most correct way to have those failures would
> be to use directly exit(EXIT_FAILURE) in file_utils.c where
> appropriate.

Yeah, there is more to do.  The reason I'm focusing on these two right
now is that they would typically run as a background service, and a
clean exit is most important there.  In the other cases, the program
runs more often in the foreground and you can see error messages.  There
are also some cases where fsync() failures are intentionally ignored
((void) casts), so some of that would need to be investigated further.

Here is a patch to get started.  Note that these calls don't go through
file_utils.c, so it's a separate issue anyway.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Jun 25, 2019 at 02:23:05PM +0200, Peter Eisentraut wrote:
> Yeah, there is more to do.  The reason I'm focusing on these two right
> now is that they would typically run as a background service, and a
> clean exit is most important there.  In the other cases, the program
> runs more often in the foreground and you can see error messages.  There
> are also some cases where fsync() failures are intentionally ignored
> ((void) casts), so some of that would need to be investigated further.

The remaining three calls all go through file_utils.c.

> Here is a patch to get started.  Note that these calls don't go through
> file_utils.c, so it's a separate issue anyway.

Why using a different error code.  Using EXIT_FAILURE is a more common
practice in the in-core binaries.  The patch looks fine to me except
that, that's a good first cut.
--
Michael

While reviewing this patch I read through some of the other fsync
callsites and noticed this typo (walkdir is in file_utils.c, not
initdb.c) too:

diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index 315c74c745..9b79df2d7f 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -3208,7 +3208,7 @@ SyncDataDirectory(void)
  *
  * Errors are reported at level elevel, which might be ERROR or less.
  *
- * See also walkdir in initdb.c, which is a frontend version of this logic.
+ * See also walkdir in file_utils.c, which is a frontend version of this logic.
  */
 static void
 walkdir(const char *path,

Regards,
-- Sehrope Sarkuni
Founder & CEO | JackDB, Inc. | https://www.jackdb.com/

On Sat, Jul 27, 2019 at 01:06:06PM -0400, Sehrope Sarkuni wrote:
> While reviewing this patch I read through some of the other fsync
> callsites and noticed this typo (walkdir is in file_utils.c, not
> initdb.c) too:

Thanks, Sehrope.  Applied.
--
Michael

On 2019-07-27 19:02, Sehrope Sarkuni wrote:
> Tried out this patch and it applies, compiles, and passes check-world.
> Also flipped things around in pg_recvlogical.c to exit-on-success to
> ensure it's actually being called and that worked too. Outside of a more
> complicated harness that simulates fsync errors not sure how else to
> test this further.

I have committed this, with the exit code changed back, as requested by
Michael.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services