Re: Refactoring the checkpointer's fsync request queue - Mailing list pgsql-hackers
From | Shawn Debnath |
---|---|
Subject | Re: Refactoring the checkpointer's fsync request queue |
Date | |
Msg-id | d28b006ddc754f6db8031fe14ba0b71d@EX13D05UWC002.ant.amazon.com Whole thread Raw |
In response to | Re: Refactoring the checkpointer's fsync request queue (Andres Freund <andres@anarazel.de>) |
List | pgsql-hackers |
On Wed, Mar 06, 2019 at 05:33:54AM +1300, Thomas Munro wrote: > On Wed, Mar 6, 2019 at 5:07 AM Shawn Debnath <sdn@amazon.com> wrote: > > Confirmed. Patch shows 8900 ms vs 192 ms on master for the insert test. > > Interesting! It's reproducible so should be able to figure out what's > > going on. The only thing we do in ForwardSyncRequest() is split up the 8 > > bits into 2x4 bits and copy the FileTagData structure to the > > checkpointer queue. Will report back what I find. Fixed - tried to be clever with a do while loop and ended up forcing a sleep of 10 ms for every register request. Silly mistake (had an assert with the right assertion right after!). Reverted to while(1) with a clean break if we meet the conditions. Thanks for Thomas for being a second set of eyes during the investigation. make check runs are happy: patch: make check 2.48s user 0.94s system 12% cpu 27.411 total master: make check 2.50s user 0.88s system 12% cpu 27.573 total > More review, all superficial stuff: > > +typedef struct > +{ > + RelFileNode rnode; > + ForkNumber forknum; > + SegmentNumber segno; > +} FileTagData; > + > +typedef FileTagData *FileTag; > > Even though I know I said we should take FileTag by pointer, and even > though there is an older tradition in the tree of having a struct > named "FooData" and a corresponding pointer typedef named "Foo", as > far as I know most people are not following the convention for new > code and I for one don't like it. One problem is that there isn't a > way to make a pointer-to-const type given a pointer-to-non-const type, > so you finish up throwing away const from your programs. I like const > as documentation and a tiny bit of extra compiler checking. What do > you think about "FileTag" for the struct and eg "const FileTag *tag" > when receiving one as a function argument? More compile time safety checks are always better. I have made the changes in this new patch. Also, followed BufferTag pattern and defined INIT_FILETAG that will initialize the structure with the correct values. This avoids the point you bring up of accidentally omitting initializing members when new ones are added. > +#include "fmgr.h" > +#include "storage/block.h" > +#include "storage/relfilenode.h" > +#include "storage/smgr.h" > +#include "storage/sync.h" > > Why do we need to include fmgr.h in md.h? Removed. > +/* md storage manager funcationality */ > > Typo. Fixed > +/* md sync callback forward declarations */ > > These aren't "forward" declarations, they're plain old declarations. Removed and simplified. > +extern char* mdfilepath(FileTag ftag); > > Doesn't really matter too much because all of this will get > pgindent-ed at some point, but FYI we write "char *md", not "char* > md". Hmm - different, noted. Changed. > #include "storage/smgr.h" > +#include "storage/md.h" > #include "utils/hsearch.h" > > Bad sorting. Ordered correctly.. > + FileTagData tag; > + tag.rnode = reln->smgr_rnode.node; > + tag.forknum = forknum; > + tag.segno = seg->mdfd_segno; > > I wonder if it would be better practice to zero-initialise that > sucker, so that if more members are added we don't leave them > uninitialised. I like the syntax "FileTagData tag = {{0}}". > (Unfortunately extra nesting required here because first member is a > struct, and C99 doesn't allow us to use empty {} like C++, even though > some versions of GCC accept it. Rats.) See comments above for re-defining FileTag. -- Shawn Debnath Amazon Web Services (AWS)
Attachment
pgsql-hackers by date: