Re: Refactoring the checkpointer's fsync request queue

On Wed, Mar 27, 2019 at 5:48 AM Shawn Debnath <sdn(at)amazon(dot)com> wrote:
> 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(dot)munro(at)gmail(dot)com> wrote:
> > > 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.

I found a few more things that I thought needed adjustment:

* Packing handler and request type into a uint8 is cute but a waste of
time if we're just going to put it in a struct next to a member that
requires word-alignment. So I changed it to a pair of plain old int16
members. The ftag member starts at offset 4 either way, on my system.

* I didn't really like the use of the word HIERARCHY in the name of
the request type, and changed it to SYNC_FILTER_REQUEST. That word
came up because we were implementing a kind of hierarchy, where if you
drop a database you want to forget things for all segments inside all
relations inside that database, but the whole point of this new API is
that it doesn't understand that, it calls a filter function to decide
which requests to keep. So I preferred "filter" as a name for the
type of request.

* I simplified the "matches" callback interface.

* Various typos and comment clean-up.

I'm going to do some more testing and tidying tomorrow (for example I
think the segment.h header is silly and I'd like to remove that), and
commit this.

--
Thomas Munro
https://enterprisedb.com