On 2021/03/18 19:19, Thomas Munro wrote:
> On Thu, Mar 18, 2021 at 8:52 PM Paul Guo <guopa@vmware.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
