Re: Refactoring the checkpointer's fsync request queue

On Thu, Apr 4, 2019 at 6:41 PM Shawn Debnath <sdn(at)amazon(dot)com> wrote:
> On Thu, Apr 04, 2019 at 05:39:14PM +1300, Thomas Munro wrote:
> > On Thu, Apr 4, 2019 at 5:36 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > > On 2019-04-03 21:19:45 -0700, Shawn Debnath wrote:
> > > > +typedef struct FileTag
> > > > +{
> > > > + int16 handler; /* SyncRequstHandler value, saving space */
> > > > + int16 forknum; /* ForkNumber, saving space */
> > > > + RelFileNode rnode;
> > > > + BlockNumber segno;
> > > > +} FileTag;
> > >
> > > Seems odd to me to use BlockNumber for segno.
> >
> > That is a tradition in md.c code. I had a new typedef SegmentNumber
> > in all sync.{c,h} stuff in an earlier version, but had trouble
> > figuring out where to define it...
>
> Thomas, this is why I had defined segment.h with the contents below :-)
>
> +++ b/src/include/storage/segment.h
> [...]
> +/*
> + * Segment Number:
> + *
> + * Each relation and its forks are divided into segments. This
> + * definition formalizes the definition of the segment number.
> + */
> +typedef uint32 SegmentNumber;
> +
> +#define InvalidSegmentNumber ((SegmentNumber) 0xFFFFFFFF)

I don't think it's project policy to put a single typedef into its own
header like that, and I'm not sure where else to put it. I have
changed FileTag's segno member to plain old uint32 for now. md.c
continues to use BlockNumber for segment numbers (as it has ever since
commit e0c9301c87 switched over from int), but that's all internal to
md.c and I think we can reasonably leave that sort of improvement to a
later patch.

Pushed. Thanks for all the work on this!

--
Thomas Munro
https://enterprisedb.com