Date: Tue, 26 Mar 2019 09:47:58 -0700
From: Shawn Debnath <sdn@amazon.com>
To: Robert Haas <robertmhaas@gmail.com>
CC: Thomas Munro <thomas.munro@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>,
        Andres Freund <andres@anarazel.de>,
        Dmitry Dolgov <9erthalion6@gmail.com>,
        PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: Refactoring the checkpointer's fsync request queue
Message-ID: <20190326164758.GA78630@f01898859afd.ant.amazon.com>
References: 
 <CA+hUKGLiUQ2mMQBTencjZKt88io5m7eguRmuxbaPqPTyL6ERtg@mail.gmail.com>
 <20190305160715.GA26468@f01898859afd.ant.amazon.com>
 <CA+hUKGJX302ctT26zXc1w7PvrwcUWHNUH0casYhA6Si0d7xDng@mail.gmail.com>
 <3127.1551804796@sss.pgh.pa.us>
 <20190305171557.GA30015@f01898859afd.ant.amazon.com>
 <CA+hUKGJ_VpGoT54=A4SCAPOFJ0Q+S5oTFNRL+HZzC-E_djCdrw@mail.gmail.com>
 <20190313010007.GB8301@f01898859afd.ant.amazon.com>
 <CA+hUKGL_sOaYRqnvbKYK5jYRx1faqrtCg8uiJ2V1DDX9BDs6Dg@mail.gmail.com>
 <CA+hUKG+6dLs2Ok1+wnq5PKLMw2N8t99wuHzoEYqod9cDWKuznw@mail.gmail.com>
 <CA+Tgmob19umNQogKHknsG+smLUZeHihNAk-h+KxPd82SDi10WQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: 
 <CA+Tgmob19umNQogKHknsG+smLUZeHihNAk-h+KxPd82SDi10WQ@mail.gmail.com>
User-Agent: Mutt/1.11.3 (2019-02-01)
Precedence: Bulk
Precedence: bulk

On Tue, Mar 26, 2019 at 09:22:56AM -0400, Robert Haas wrote:
> On Tue, Mar 26, 2019 at 12:20 AM Thomas Munro <thomas.munro@gmail.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.

Makes sense for now. When we re-visit the fd-passing or sync-on-close 
implementations, we can adapt the changes relatively easily given the 
rest of the framework is staying intact. I am hoping these patches do 
not delay the last fsync-gate issue discussion further.  

-- 
Shawn Debnath
Amazon Web Services (AWS)