Обсуждение: Re: 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