On 2020/01/24 16:56, Julien Rouhaud wrote:
> On Fri, Jan 24, 2020 at 8:20 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>
>> On 2020/01/24 15:38, Arthur Zakirov wrote:
>>> On 2020/01/24 14:56, Michael Paquier wrote:
>>>> On Fri, Jan 24, 2020 at 01:28:29PM +0900, Arthur Zakirov wrote:
>>>>> It is compiled and passes the tests. There is the documentation and
>>>>> it is
>>>>> built too without an error.
>>>>>
>>>>> It seems that consensus about the returned type was reached and I
>>>>> marked the
>>>>> patch as "Ready for Commiter".
>>>>
>>>> + fsync_fname_ext(filename, S_ISDIR(fst.st_mode), false, ERROR);
>>>> One comment here: should we warn better users in the docs that a fsync
>>>> failule will not trigger a PANIC here? Here, fsync failure on heap
>>>> file => ERROR => potential data corruption.
>>>
>>> Ah, true. It is possible to add couple sentences that pg_file_sync()
>>> doesn't depend on data_sync_retry GUC and doesn't raise a PANIC even for
>>> database files.
>>
>> Thanks all for the review!
>>
>> So, what about the attached patch?
>> In the patch, I added the following note to the doc.
>>
>> --------------------
>> Note that
>> <xref linkend="guc-data-sync-retry"/> has no effect on this function,
>> and therefore a PANIC-level error will not be raised even on failure to
>> flush database files.
>> --------------------
>
> We should explicitly mention that this can cause corruption. How about:
>
> --------------------
> Note that
> <xref linkend="guc-data-sync-retry"/> has no effect on this function,
> and therefore a PANIC-level error will not be raised even on failure to
> flush database files. If that happens, the underlying database
> objects may be corrupted.
> --------------------
IMO that's overkill. If we really need such mention for pg_file_sync(),
we also need to add it for other functions like pg_read_file(),
pg_stat_file(), etc. But, again, which looks overkill.
Regards,
--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters
