Re: Add pg_file_sync() to adminpack

On Fri, Jan 10, 2020 at 8:16 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Fri, Jan 10, 2020 at 06:50:12PM +0900, Fujii Masao wrote:
> > I changed the doc that way. Thanks for the review!

Thanks for the review!

> + <para>
> + <function>pg_file_sync</function> fsyncs the specified file or directory
> + named by <parameter>filename</parameter>. Returns true on success,
> + an error is thrown otherwise (e.g., the specified file is not present).
> + </para>
> What's the point of having a function that returns a boolean if it
> just returns true all the time? Wouldn't it be better to have a set
> of semantics closer to the unlink() part, where the call of stat()
> fails with an ERROR for (errno != ENOENT) and the fsync call returns
> false with a WARNING?

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.

> +SELECT pg_file_sync('global'); -- sync directory
> + pg_file_sync
> +--------------
> + t
> +(1 row)
> installcheck deployments may not like that.

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?

Regards,

--
Fujii Masao