On 3/19/21 1:28 AM, Thomas Munro wrote:
> On Fri, Mar 19, 2021 at 3:23 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>> Thanks for updating the patch! It looks good to me!
>> I have one minor comment for the patch.
>>
>> + elog(LOG, "could not open %s: %m", path);
>> + return;
>> + }
>> + if (syncfs(fd) < 0)
>> + elog(LOG, "could not sync filesystem for \"%s\": %m", path);
>>
>> Since these are neither internal errors nor low-level debug messages, ereport() should be used for them rather than
elog()?For example,
>
> Fixed.
>
> I'll let this sit until tomorrow to collect any other feedback or
> objections, and then push the 0001 patch
> (recovery_init_sync_method=syncfs).
I had a look at the patch and it looks good to me.
Should we mention in the docs that the contents of non-standard symlinks
will not be synced, i.e. anything other than tablespaces and pg_wal? Or
can we point them to some docs saying not to do that (if such exists)?
> On Fri, Mar 19, 2021 at 4:08 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>> 0002 patch looks good to me. Thanks!
>> I have minor comments.
>
> Ok, I made the changes you suggested. Let's see if anyone else would
> like to vote for or against the concept of the 0002 patch
> (recovery_init_sync_method=none).
It worries me that this needs to be explicitly "turned off" after the
initial recovery. Seems like something of a foot gun.
Since we have not offered this functionality before I'm not sure we
should rush to introduce it now. For backup solutions that do their own
syncing, syncfs() should provide excellent performance so long as the
file system is not shared, which is something the user can control (and
is noted in the docs).
Regards,
--
-David
david@pgmasters.net
