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

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Postgres hackers <pgsql-hackers(at)postgresql(dot)org>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>
Subject: Re: pgsql: Fix failure to check for open() or fsync() failures.
Date: 2018-12-26 22:43:51
Message-ID: 20181226224351.GA2106@paquier.xyz
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-committers pgsql-hackers

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

In response to

Responses

Browse pgsql-committers by date

  From Date Subject
Next Message Tom Lane 2018-12-26 22:55:36 Re: pgsql: Fix failure to check for open() or fsync() failures.
Previous Message Tom Lane 2018-12-26 21:08:23 pgsql: Fix failure to check for open() or fsync() failures.

Browse pgsql-hackers by date

  From Date Subject
Next Message Noah Misch 2018-12-26 22:44:28 Re: Move regression.diffs of pg_upgrade test suite
Previous Message Tomas Vondra 2018-12-26 22:09:05 Re: [HACKERS] PATCH: multivariate histograms and MCV lists