On Sat, Jun 4, 2016 at 12:41 PM, Robert Haas <[email protected]> wrote: > On Fri, Jun 3, 2016 at 10:25 PM, Masahiko Sawada <[email protected]> > 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?
Yeah, you're right. the rewriteVisibilityMap() always exactly writes whole new_vmbuf. > >> 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). I specified O_EXCL instead of O_TRUNC. Attached updated patch. Regards, -- Masahiko Sawada
fix_freeze_map_7087166_v2.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
