Re: Refactoring the checkpointer's fsync request queue

Hi,

Thanks for the update!

On 2019-02-20 15:27:40 -0800, Shawn Debnath wrote:
> As promised, here's a patch that addresses the points discussed by
> Andres and Thomas at FOSDEM. As a result of how we want checkpointer to
> track what files to fsync, the pending ops table now integrates the
> forknum and segno as part of the hash key eliminating the need for the
> bitmapsets, or vectors from the previous iterations. We re-construct the
> pathnames from the RelFileNode, ForkNumber and SegmentNumber and use
> PathNameOpenFile to get the file descriptor to use for fsync.

I still object to exposing segment numbers to smgr and above. I think
that's an implementation detail of the various storage managers, and we
shouldn't expose smgr.c further than it already is.

> Apart from that, this patch moves the system for requesting and
> processing fsyncs out of md.c into smgr.c, allowing us to call on smgr
> component specific callbacks to retrieve metadata like relation and
> segment paths. This allows smgr components to maintain how relfilenodes,
> forks and segments map to specific files without exposing this knowledge
> to smgr. It redefines smgrsync() behavior to be closer to that of
> smgrimmedsysnc(), i.e., if a regular sync is required for a particular
> file, enqueue it in locally or forward it to checkpointer.
> smgrimmedsync() retains the existing behavior and fsyncs the file right
> away. The processing of fsync requests has been moved from mdsync() to a
> new ProcessFsyncRequests() function.

I think that's also wrong, imo fsyncs are storage detail, and should be
relegated to *below* md.c. That's not to say the code should live in
md.c, but the issuing of such calls shouldn't be part of smgr.c -
consider e.g. developing a storage engine living in non volatile ram.

Greetings,

Andres Freund