On Fri, Jun 3, 2016 at 10:25 PM, Masahiko Sawada <sawada.m...@gmail.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 -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers