Re: Refactoring the checkpointer's fsync request queue

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