On Sat, May 7, 2016 at 5:42 AM, Robert Haas <[email protected]> wrote: > On Thu, May 5, 2016 at 2:20 PM, Andres Freund <[email protected]> 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
fix_freeze_map_7087166.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
