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