Date: Wed, 30 Jan 2019 21:59:38 -0800
From: Shawn Debnath <sdn@amazon.com>
To: Thomas Munro <thomas.munro@enterprisedb.com>
CC: Dmitry Dolgov <9erthalion6@gmail.com>, Robert Haas
	<robertmhaas@gmail.com>, Andres Freund <andres@anarazel.de>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: Refactoring the checkpointer's fsync request queue
Message-ID: <20190131055938.GA52540@f01898859afd.ant.amazon.com>
References: <20181114214948.r2adyxjclnz2lvxe@alap3.anarazel.de>
 <CA+TgmoZRNHDKSnG35mPufKKgVXSPAYrQo5nHhD_ZzBzg50cTAQ@mail.gmail.com>
 <CAEepm=3c6ybA7zUCQEuFees7o6JGhNGdS3V7ay3EdCU6avDaaA@mail.gmail.com>
 <CA+TgmobJDfSZ2x_vic5ZoheVcJBn-AmnndUs_99vzP9VStnBuA@mail.gmail.com>
 <CAEepm=10A5eySmc9=sPje6nNbaSBYB0gQ3Rb0dDvph-Ei2H0cQ@mail.gmail.com>
 <CAEepm=1TDor1xHkYtpqEpuu5kpoU-DbLGX49scCY_zY-t3H-ug@mail.gmail.com>
 <CA+q6zcV=p=1G2Z8itJ9=tAn=b8ha3pO4RQCMt9YXOEutRV33YQ@mail.gmail.com>
 <CAEepm=2qBbQXGeekhCY+W8m=fy-+eU_FAUzRCSPFKuZ2rKG1LQ@mail.gmail.com>
 <CAEepm=1uqZLcTqLz_+egZRYotnTvzDK-jOGZdN8QhL=7nixY5Q@mail.gmail.com>
 <CAEepm=1mp+7PPY+sKJ4cvs6hba0NnWeed00oJ=EWry0gUgnJ_A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: 
 <CAEepm=1mp+7PPY+sKJ4cvs6hba0NnWeed00oJ=EWry0gUgnJ_A@mail.gmail.com>
User-Agent: Mutt/1.11.2 (2019-01-07)
Precedence: Bulk
Precedence: bulk

I (finally) got a chance to go through these patches and they look 
great. Thank you for working on this!  Few comments:

- I do not see SmgrFileTag being defined or used like you mentioned in 
  your first email. I see RelFileNode still being used. Is this planned 
  for the future?
- Would be great to add a set of Tests for SimpleVector.

> For the 0001 patch, I'll probably want to reconsider the naming a it  
> ("simple -> "specialized", "generic", ...?)

I think the name SimpleVector is fine, fits with the SimpleHash theme.  
If the goal is to shorten it, perhaps PG prefix would suffice?

> 4. The protocol for forgetting relations etc is slightly different:
> if a file is found to be missing, AbsortFsyncRequests() and then probe
> to see if the segment number disappeared from the set (instead of
> cancel flags), though I need to test this case.

Can you explain this part a bit more? I am likely missing something in 
the patch.

> I couldn't resist the urge to try porting pg_qsort() to this style.
> It seems to be about twice as fast as the original at sorting integers
> on my machine with -O2.  I suppose people aren't going to be too
> enthusiastic about yet another copy of qsort in the tree, but maybe
> this approach (with a bit more work) could replace the Perl code-gen
> for tuple sorting.  Then the net number of copies wouldn't go up, but
> this could be used for more things too, and it fits with the style of
> simplehash.h and simplevector.h.  Thoughts?

+1 for avoiding duplicate code. Would it be acceptable to migrate the 
rest of the usages to this model over time perhaps? Love to move this 
patch forward.

I wonder if it might be better to introduce two different functions 
catering to the two different use cases for forcing an immediate sync:

- sync a relation
    smgrimmedsyncrel(SMgrRelation, ForkNumber)
- sync a specific segment
    smgrimmedsyncseg(SMgrRelation, ForkNumber, SegmentNumber)

This will avoid having to specify InvalidSegmentNumber for majority of 
the callers today.

-- 
Shawn Debnath
Amazon Web Services (AWS)