Re: Reviewing freeze map code

On Sat, Jun 4, 2016 at 12:41 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Jun 3, 2016 at 10:25 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>>> + char new_vmbuf[BLCKSZ];
>>>> + char *new_cur = new_vmbuf;
>>>> + bool empty = true;
>>>> + bool old_lastpart;
>>>> +
>>>> + /* Copy page header in advance */
>>>> + memcpy(new_vmbuf, &pageheader, SizeOfPageHeaderData);
>>>>
>>>> Shouldn't we zero out new_vmbuf? Afaics we're not necessarily zeroing it
>>>> with old_lastpart && !empty, right?
>>>
>>> Oh, dear. That seems like a possible data corruption bug. Maybe we'd
>>> better fix that right away (although I don't actually have time before
>>> the wrap).
>
> Actually, on second thought, I'm not seeing the bug here. It seems to
> me that the loop commented this way:
>
> /* Process old page bytes one by one, and turn it into new page. */
>
> ...should always write to every byte in new_vmbuf, because we process
> exactly half the bytes in the old block at a time, and so that's going
> to generate exactly one full page of new bytes. Am I missing
> something?

Yeah, you're right.
the rewriteVisibilityMap() always exactly writes whole new_vmbuf.

>
>> Since the force is always set true, I removed the force from argument
>> of copyFile() and rewriteVisibilityMap().
>> And destination file is always opened with O_RDWR, O_CREAT, O_TRUNC flags .
>
> I'm not happy with this. I think we should always open with O_EXCL,
> because the new file is not expected to exist and if it does,
> something's probably broken. I think we should default to the safe
> behavior (which is failing) rather than the unsafe behavior (which is
> clobbering data).

I specified O_EXCL instead of O_TRUNC.

Attached updated patch.

Regards,

--
Masahiko Sawada