On Wed, Mar 13, 2019 at 2:27 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> [...] Aside from some refactoring which I
> think looks good anyway and prepares for future patches, the main
> effect of this patch is to force the checkpointer to open and close
> the files every time which seems OK to me.
I've been trying to decide if that is a problem. Maybe there is a
performance angle, and I'm also wondering if it might increase the
risk of missing a write-back error. Of course we'll find a proper
solution to that problem (perhaps fd-passing or sync-on-close[1]), but
I don't want to commit anything in the name of refactoring that might
make matters worse incidentally. Or perhaps those worries are bogus,
since the checkpointer calls smgrcloseall() at the end anyway.
On balance, I'm inclined to err on the side of caution and try to keep
things a bit closer to the way they are for now.
Here's a fixup patch. 0001 is the same as Shawn's v12 patch, and 0002
has my proposed changes to switch to callbacks that actually perform
the sync and unlink operations given a file tag, and do so via the
SMGR fd cache, rather than exposing the path to sync.c. This moves us
back towards the way I had it in an earlier version of the patch, but
instead of using smgrsyncimmed() as I had it, it goes via Shawn's new
sync handler function lookup table, allowing for non-smgr components
to use this machinery in future (as requested by Andres).
Thoughts?
It all needs to be pgindented, I'll do that later. I'll post a rebase
of my undo stuff on top of this soon, to show how it looks with this
interface.
[1]
https://www.postgresql.org/message-id/flat/CA%2BhUKGLMPXMnSLDwgnNRFPyxvf_0bJ5HwXcHWjCp7Cvh7G%3DxEA%40mail.gmail.com
--
Thomas Munro
https://enterprisedb.com
