Re: Add pg_file_sync() to adminpack

On 2020/01/13 22:46, Michael Paquier wrote:
> On Sat, Jan 11, 2020 at 02:12:15AM +0900, Fujii Masao wrote:
>> I'm not sure if returning false with WARNING only in some error cases
>> is really good idea or not. At least for me, it's more intuitive to
>> return true on success and emit an ERROR otherwise. I'd like to hear
>> more opinions about this. Also if returning true on success is rather
>> confusing, we can change its return type to void.
>
> An advantage of not issuing an ERROR if that when working on a list of
> files (for example a WITH RECURSIVE on the whole data directory?), you
> can then know which files could not be synced instead of seeing one
> ERROR about one file, while being unsure about the state of the
> others.
>
>> Could you elaborate why? But if it's not good to sync the existing directory
>> in the regression test, we may need to give up testing the sync of directory.
>> Another idea is to add another function like pg_mkdir() into adminpack
>> and use the directory that we newly created by using that function,
>> for the test. Or better idea?
>
> We should avoid potentially costly tests in any regression scenario if
> we have a way to do so. I like your idea of having a pg_mkdir(), that
> feels more natural to have as there is already pg_file_write().

BTW, in the latest patch that I posted upthread, I changed
the directory to sync for the test from "global" to "pg_stat"
because pg_stat is empty while the server is running,
and syncing it would not be so costly.
Introduing pg_mkdir() (maybe pg_rmdir() would be also necessary)
is an idea, but it's better to do that as a separate patch
if it's really necessary.

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters