Re: Refactoring the checkpointer's fsync request queue

On Fri, Feb 22, 2019 at 03:57:42PM -0800, Andres Freund wrote:
> > Today, all the callbacks for smgr have their prototypes defined in
> > smgr.h and used in smgr.c. Forward declarations within the new sync.h
> > would be fine but having md callbacks split in two different places may
> > not be the cleanest approach? One could argue they serve different
> > purposes so perhaps it correct that we define them separately. I am
> > fine with either, but now I probably prefer the new enum to fixed
> > function declaration mappings that Andres suggested. I agree it would be
> > less error prone.
>
> I'd really just entirely separate the two. I'd not include any knowledge
> of this mechanism into smgr.h, and just make the expansion of paths
> dispatch statically dispatched (potentially into md.c) to actually do
> the work. I'm not sure why sync.h would need any forward declarations
> for that?

We had a quick offline discussion to get on the same page and we agreed
to move forward with Andres' approach above. Attached is patch v10.
Here's the overview of the patch:

1. Move the system for requesting and processing fsyncs out of md.c
into storage/sync/sync.c with definitions in include/storage/sync.h.
ProcessSyncRequests() is now responsible for processing the sync
requests during checkpoint.

2. Removed the need for specific storage managers to implement pre and
post checkpoint callbacks. These are now executed by the sync mechanism.

3. We now embed the fork number and the segment number as part of the
hash key for the pending ops table. This eliminates the bitmapset based
segment tracking for each relfilenode during fsync as not all storage
managers may map their segments from zero.

4. Each sync request now must include a type: sync, forget, forget
hierarchy, or unlink, and the owner who will be responsible for
generating paths or matching forget requests.

5. For cancelling relation sync requests, we now must send a forget
request for each fork and segment in the relation.

6. We do not rely on smgr to provide the file descriptor we use to
issue fsync. Instead, we generate the full path based on the FileTag
in the sync request and use PathNameOpenFile to get the file descriptor.

Ran make check-world and repeated the tests described in [1]. The
numbers show a 12% drop in total time for single run of 1000 clients and
~62% drop in total time for 10 parallel runs with 200 clients:

[Requests Absorbed]

Min Max Average Median Std Dev
--------------- -------- -------- ----------- --------- ----------
patch-1x1000 17554 147410 85252.95 83835.5 21898.88
master-1x1000 25728 138422 81455.04 80601 21295.83

Min Max Average Median Std Dev
patch-10x200 125922 279682 197098.76 197055 34038.25
master-10x200 191833 602512 416533.86 424946 82014.48

[Files Synced]

Min Max Average Median Std Dev
--------------- ------ ------ --------- -------- ---------
patch-1x1000 155 213 158.93 159 2.97
master-1x1000 154 166 158.29 159 10.29

Min Max Average Median Std Dev
patch-10x200 1456 1577 1546.84 1547 11.23
master-10x200 1546 1546 1546 1559 12.79

[Total Time in ProcessFsyncRequest/mdsync]

Min Max Average Median Std Dev
--------------- ------ --------- ---------- -------- ---------
patch-1x1000 606 4022 2145.11 2123 440.72
master-1x1000 806 4430.32 2458.77 2382 497.01

Min Max Average Median Std Dev
patch-10x200 2472 6960 4156.8 4141 817.56
master-10x200 4323 17858 10982.15 11154 2760.47

[1]
https://www.postgresql.org/message-id/20190220232739.GA8280%40f01898859afd.ant.amazon.com

--
Shawn Debnath
Amazon Web Services (AWS)