Re: Refactoring the checkpointer's fsync request queue

On Sat, Mar 2, 2019 at 8:36 AM Shawn Debnath <sdn@amazon.com> wrote:
> On Fri, Mar 01, 2019 at 01:15:21PM -0500, Robert Haas wrote:
> > > >
> > > > +typedef enum syncrequestowner
> > > > +{
> > > > +       SYNC_MD = 0             /* md smgr */
> > > > +} syncrequestowner;
> > > >
> > > > I have a feeling that Andres wanted to see a single enum combining
> > > > both the "operation" and the "owner", like SYNC_REQ_CANCEL_MD,
> > > > SYNC_REQ_CANCEL_UNDO, ... but I actually like it better the way you
> > > > have it.
> > >
> > > Obviously it's nicer looking this way, but OTOH, that means we have to
> > > send more data over the queue, because we can't easily combine the
> > > request + "owner". I don't have too strong feelings about it though.
> >
> > Yeah, I would lean toward combining those.
>
> I disagree, at least with combining and retaining enums. Encoding all
> the possible request types with the current, planned and future SMGRs
> would cause a sheer explosion in the number of enum  values. Not to
> mention that you have multiple enum values for the same behavior - which
> just isn't clean. And handling of these enums in the code would be ugly
> too.
>
> Do note that these are typedef'ed to uint8 currently. For a default
> config with 128 MB shared_buffers, we will use an extra 16kB (one extra
> byte to represent the owner).  I am hesitant to change this right now
> unless folks feel strongly about it.
>
> If so, I would combine the type and owner by splitting it up in 4 bit
> chunks, allowing for 16 request types and 16 smgrs. This change would
> only apply for the in-memory queue. The code and functions would
> continue using the enums.

+1

-- 
Thomas Munro
https://enterprisedb.com