Re: Reviewing freeze map code

On Sat, Jun 4, 2016 at 12:41 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, Jun 3, 2016 at 10:25 PM, Masahiko Sawada <sawada.mshk@gmail.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