Re: Reviewing freeze map code

On Sat, May 7, 2016 at 5:42 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, May 5, 2016 at 2:20 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>> On 2016-05-02 14:48:18 -0700, Andres Freund wrote:
>>> 7087166 pg_upgrade: Convert old visibility map format to new format.
>>
>> +const char *
>> +rewriteVisibilityMap(const char *fromfile, const char *tofile, bool force)
>> ...
>>
>> + while ((bytesRead = read(src_fd, buffer, BLCKSZ)) == BLCKSZ)
>> + {
>> ..
>>
>> Uh, shouldn't we actually fail if we read incompletely? Rather than
>> silently ignoring the problem? Ok, this causes no corruption, but it
>> indicates that something went significantly wrong.
>
> Sure, that's reasonable.
>

Fixed.

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

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 .

>> + if ((dst_fd = open(tofile, O_RDWR | O_CREAT | (force ? 0 : O_EXCL), S_IRUSR | S_IWUSR)) < 0)
>> + {
>> + close(src_fd);
>> + return getErrorText();
>> + }
>>
>> I know you guys copied this, but what's the force thing about?
>> Expecially as it's always set to true by the callers (i.e. what is the
>> parameter even about?)? Wouldn't we at least have to specify O_TRUNC in
>> the force case?
>
> I just work here.
>
>> + old_cur += BITS_PER_HEAPBLOCK_OLD;
>> + new_cur += BITS_PER_HEAPBLOCK;
>>
>> I'm not sure I'm understanding the point of the BITS_PER_HEAPBLOCK_OLD
>> stuff - as long as it's hardcoded into rewriteVisibilityMap() we'll not
>> be able to have differing ones anyway, should we decide to add a third
>> bit?
>
> I think that's just a matter of style.

So this comments is not incorporated.

Attached patch, please review it.

Regards,

--
Masahiko Sawada