From: | Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> |
---|---|
To: | Julien Rouhaud <rjuju123(at)gmail(dot)com> |
Cc: | Arthur Zakirov <zaartur(at)gmail(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>, Robert Haas <robertmhaas(at)gmail(dot)com>, Atsushi Torikoshi <atorik(at)gmail(dot)com>, Fujii Masao <masao(dot)fujii(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Add pg_file_sync() to adminpack |
Date: | 2020-01-24 11:50:16 |
Message-ID: | 6d5ee46c-5233-eb72-2866-062b5652edfe@oss.nttdata.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 2020/01/24 17:08, Fujii Masao wrote:
>
>
> On 2020/01/24 16:56, Julien Rouhaud wrote:
>> On Fri, Jan 24, 2020 at 8:20 AM Fujii Masao
>> <masao(dot)fujii(at)oss(dot)nttdata(dot)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.
I pushed the v5 of the patch. Thanks all for reviewing the patch!
If the current document is not good yet, let's keep discussing that!
Regards,
--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters
From | Date | Subject | |
---|---|---|---|
Next Message | Pavel Stehule | 2020-01-24 12:15:13 | Re: [Proposal] Global temporary tables |
Previous Message | Alvaro Herrera | 2020-01-24 11:44:41 | Re: Allow to_date() and to_timestamp() to accept localized names |