Re: fdatasync performance problem with large number of DB files

On 2021/03/18 19:19, Thomas Munro wrote:
> On Thu, Mar 18, 2021 at 8:52 PM Paul Guo <guopa(at)vmware(dot)com> wrote:
>> About the syncfs patch, my first impression on the guc name sync_after_crash
>> is that it is a boolean type. Not sure about other people's feeling. Do you guys think
>> It is better to rename it to a clearer name like sync_method_after_crash or others?
>
> Works for me. Here is a new version like that, also including the
> documentation change discussed with Fujii-san, and a couple of
> cosmetic changes.

Thanks for updating the patch!

+ database cluster that did not shut down cleanly, including copies
+ created with pg_basebackup.

"pg_basebackup" should be "<application>pg_basebackup</application>"?

+ while ((de = ReadDir(dir, "pg_tblspc")))

The comment of SyncDataDirectory() says "Errors are logged but not
considered fatal". So ReadDirExtended() with LOG level should be used
here, instead?

Isn't it better to call CHECK_FOR_INTERRUPTS() in this loop?

+ fd = open(path, O_RDONLY);

For current use, it's not harmful to use open() and close(). But isn't
it safer to use OpenTransientFile() and CloseTransientFile(), instead?
Because do_syncfs() may be used for other purpose in the future.

+ if (syncfs(fd) < 0)
+ elog(LOG, "syncfs failed for %s: %m", path);

According to the message style guide, this message should be something
like "could not sync filesystem for \"%s\": %m"?

I confirmed that no error was reported when crash recovery started with
syncfs, in old Linux. I should also confirm that no error is reported in that
case in Linux 5.8+, but I don't have that environement. So I've not tested
this feature in Linux 5.8+....

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION