From: | Shawn Debnath <sdn(at)amazon(dot)com> |
---|---|
To: | Thomas Munro <thomas(dot)munro(at)gmail(dot)com> |
Cc: | Andres Freund <andres(at)anarazel(dot)de>, Dmitry Dolgov <9erthalion6(at)gmail(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Refactoring the checkpointer's fsync request queue |
Date: | 2019-03-06 02:29:59 |
Message-ID: | d28b006ddc754f6db8031fe14ba0b71d@EX13D05UWC002.ant.amazon.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | 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(at)amazon(dot)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 | Content-Type | Size |
---|---|---|
0001-Refactor-the-fsync-machinery-to-support-future-SMGR-v12.patch | text/plain | 78.6 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Andres Freund | 2019-03-06 02:33:54 | Re: Pluggable Storage - Andres's take |
Previous Message | David Rowley | 2019-03-06 02:29:44 | Re: Update does not move row across foreign partitions in v11 |