Re: Refactoring the checkpointer's fsync request queue

On Tue, Mar 26, 2019 at 12:20 AM Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
> 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).

Strong +1. Not only might closing and reopening the files have
performance and reliability implications, but a future smgr might talk
to the network, having no local file to sync.

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