On 2015-12-03 22:09:43 +0100, Andres Freund wrote:
> On 2015-11-20 16:11:15 +0900, Michael Paquier wrote:
> > + if (bkpb.fork == INIT_FORKNUM)
> > + {
> > + SMgrRelation srel;
> > + srel = smgropen(bkpb.node, InvalidBackendId);
> > + smgrimmedsync(srel, INIT_FORKNUM);
> > + smgrclose(srel);
> > + }
>
> A smgrwrite() instead of a smgrimmedsync() should be sufficient here.
More importantly, the smgrimmedsync() won't actually achieve anything -
RestoreBackupBlockContents() will just have written the data into shared
buffers. smgrimmedsync() doesn't flush that.
And further, just flushing the buffer directly to disk using
smgrwrite(), without reflecting that in the in-memory state, doesn't
seem prudent. At the very least we'll write all those blocks twice. It
also likely misses dealing with checksums and such.
What I think we really ought to do instead is to use "proper" buffer
functionality, e.g. flush the buffer via FlushBuffer().
It seems slightly better not to directly expose FlushBuffer() and
instead add a tiny wrapper. Couldn't come up with a grand name tho.
For me the attached, preliminary, patch, fixes the problem in master;
previous branches ought to look mostly similar, except the flush moved
to RestoreBackupBlockContents/RestoreBackupBlock.
Does anybody have a better idea? Suitable for the back-branches?
I'm kinda wondering if it wouldn't have been better to go through shared
buffers in ResetUnloggedRelationsInDbspaceDir() instead of using
copy_file().
Regareds,
Andres
