Re: Reviewing freeze map code

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.

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

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

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