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
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. |
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 |