Re: Reviewing freeze map code

On Sat, May 7, 2016 at 5:42 AM, Robert Haas  wrote:
> On Thu, May 5, 2016 at 2:20 PM, Andres Freund  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