Re: Reviewing freeze map code

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?

> 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).

(Status update for Noah: I expect Masahiko Sawada will respond
quickly, but if not I'll give some kind of update by Monday COB
anyhow.)

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company