Hi, On 2019-03-01 23:17:27 +1300, Thomas Munro wrote: > @@ -8616,7 +8617,7 @@ CreateCheckPoint(int flags) > * the REDO pointer. Note that smgr must not do anything that'd have > to > * be undone if we decide no checkpoint is needed. > */ > - smgrpreckpt(); > + PreCheckpoint(); > > I would call this and the "post" variant something like > SyncPreCheckpoint(). Otherwise it's too general sounding and not > clear which module it's in.
Definitely. > +typedef enum syncrequesttype > +{ > + SYNC_REQUEST, > + FORGET_REQUEST, > + FORGET_HIERARCHY_REQUEST, > + UNLINK_REQUEST > +} syncrequesttype; > > These names are too generic for the global C namespace; how about > SYNC_REQ_CANCEL or similar? > > +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. FWIW, I don't like the name owner here. Class? Method? Greetings, Andres Freund