pgsql: Fix failure to check for open() or fsync() failures.

On Wed, Dec 26, 2018 at 09:08:23PM +0000, Tom Lane wrote:

Fix failure to check for open() or fsync() failures.

While it seems OK to not be concerned about fsync() failure for a
pre-existing signal file, it's not OK to not even check for open()
failure. This at least causes complaints from static analyzers,
and I think on some platforms passing -1 to fsync() or close() might
trigger assertion-type failures. Also add (void) casts to make clear
that we're ignoring fsync's result intentionally.

Oversights in commit 2dedf4d9a, noted by Coverity.

 fd = BasicOpenFilePerm(STANDBY_SIGNAL_FILE, O_RDWR | PG_BINARY | get_sync_bit(sync_method),
                        S_IRUSR | S_IWUSR);
-   pg_fsync(fd);
-   close(fd);
+   if (fd >= 0)
+   {
+       (void) pg_fsync(fd);
+       close(fd);
+   }

Wouldn't it be more simple to remove stat() and just call
BasicOpenFilePerm, complaining with FATAL about any failures,
including EACCES, on the way? The code is racy as designed, even if
that's not a big deal for recovery purposes.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

On Wed, Dec 26, 2018 at 09:08:23PM +0000, Tom Lane wrote:

Fix failure to check for open() or fsync() failures.

While it seems OK to not be concerned about fsync() failure for a
pre-existing signal file, it's not OK to not even check for open()
failure. This at least causes complaints from static analyzers,

Wouldn't it be more simple to remove stat() and just call
BasicOpenFilePerm, complaining with FATAL about any failures,
including EACCES, on the way? The code is racy as designed, even if
that's not a big deal for recovery purposes.

It appears to me that the code is intentionally not worrying about
fsync failure, so it seems wrong for it to FATAL out if it's unable
to open the file to fsync it. And it surely shouldn't do so if the
file isn't there.

regards, tom lane

On Wed, Dec 26, 2018 at 05:55:36PM -0500, Tom Lane wrote:

It appears to me that the code is intentionally not worrying about
fsync failure, so it seems wrong for it to FATAL out if it's unable
to open the file to fsync it. And it surely shouldn't do so if the
file isn't there.

My point is a bit different though: it seems to me that we could just
call BasicOpenFilePerm() and remove the stat() to do exactly the same
things, simplifying the code.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

On Wed, Dec 26, 2018 at 05:55:36PM -0500, Tom Lane wrote:

It appears to me that the code is intentionally not worrying about
fsync failure, so it seems wrong for it to FATAL out if it's unable
to open the file to fsync it. And it surely shouldn't do so if the
file isn't there.

My point is a bit different though: it seems to me that we could just
call BasicOpenFilePerm() and remove the stat() to do exactly the same
things, simplifying the code.

Oh, I see. Yeah, if we're ignoring errors anyway, the stat calls
seem redundant.

regards, tom lane

On Wed, Dec 26, 2018 at 08:35:22PM -0500, Tom Lane wrote:

Oh, I see. Yeah, if we're ignoring errors anyway, the stat calls
seem redundant.

For this one, I think that we could simplify as attached (this causes
open() to fail additionally because of the sync flags, but that's not
really worth worrying). Thoughts?
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

On Wed, Dec 26, 2018 at 08:35:22PM -0500, Tom Lane wrote:

Oh, I see. Yeah, if we're ignoring errors anyway, the stat calls
seem redundant.

For this one, I think that we could simplify as attached (this causes
open() to fail additionally because of the sync flags, but that's not
really worth worrying). Thoughts?

Actually, now that I think a bit more, this isn't a good idea. We want
standby_signal_file_found (resp. recovery_signal_file_found) to get set
if the file exists, even if we're unable to fsync it for some reason.
A counterexample to the proposed patch is that a signal file that's
read-only to the server will get ignored, which it should not be.

regards, tom lane