Re: Refactoring the checkpointer's fsync request queue

On 2019-02-22 15:45:50 -0800, Shawn Debnath wrote:
> > > Well even if you do it with individual segment cancel messages for
> > > relations, you still need a way to deal with whole-database drops
> > > (generating the cancels for every segment in every relation in the
> > > database would be nuts), and that means either exposing some structure
> > > to the requests, right?  So the requests would have { request type,
> > > callback ID, db, opaque tag }, where request type is SYNC, CANCEL,
> > > CANCEL_WHOLE_DB, callback ID is used to find the function that
> > > converts opaque tags to paths, and db is used for handling
> > > CANCEL_WHOLE_DB requests where you need to scan the whole hash table.
> > > Right?
> >
> > I'm ok with using callbacks to allow pruning for things like droping
> > databases. If we use callbacks, I don't see a need to explicitly include
> > the db in the request however? The callback can look into the opaque
> > tag, no?  Also, why do we need a separation between request type and
> > callback? That seems like it'll commonly be entirely redundant?
>
> By having the id to distinguish which smgr to use for callbacks, we
> don't need DB. You are correct, my plan was to make the whole request
> opaque to the sync mechanism. ForwardFsyncRequest will take requester
> smgr id, request type (forget [db/rel], sync), relfilenode, forknum, and
> segno and convert it into a opaque CheckpointRequest and queue it
> locally. The only responsibility here is to map the different pieces of
> data into the opaque CheckpointerRequest. Requester ID in combination
> with request type will help us look up which callback function to
> execute.
>
> A new enum for requester ID is perfectly okay. I was trying to recycle
> the smgr id but perhaps that's not the right approach.

I think it'd be a bad idea to use the smgr id here. So +1 for
separating.


> > > > > Yeah I suggested dynamic registration to avoid the problem that
> > > > > eg
> > > > > src/backend/storage/sync.c otherwise needs to forward declare
> > > > > md_tagtopath(), undofile_tagtopath(), slru_tagtopath(), ..., or maybe
> > > > > #include <storage/md.h> etc, which seemed like exactly the sort of
> > > > > thing up with which you would not put.
> > > >
> > > > I'm not sure I understand. If we have a few known tag types, what's the
> > > > problem with including the headers with knowledge of how to implement
> > > > them into sync.c file?
> > >
> > > Typo in my previous email: src/backend/storage/file/sync.c was my
> > > proposal for the translation unit holding this stuff (we don't have .c
> > > files directly under storage).  But it didn't seem right that stuff
> > > under storage/file (things concerned with files) should know about
> > > stuff under storage/smgr (md.c functions, higher level smgr stuff).
> > > Perhaps that just means it should go into a different subdir, maybe
> > > src/backend/storage/sync/sync.c, that knows about files AND smgr
> > > stuff, or perhaps I'm worrying about nothing.
> >
> > I mean, if you have a md_tagtopath and need its behaviour, I don't
> > understand why a local forward declaration changes things?  But I also
> > think this is a bit of a non-problem - the abbreviated path names are
> > just a different representation of paths, I don't see a problem of
> > having that in sync.[ch], especially if we have the option to also have
> > a full-length pathname variant if we ever need that.
>
> Today, all the callbacks for smgr have their prototypes defined in
> smgr.h and used in smgr.c.  Forward declarations within the new sync.h
> would be fine but having md callbacks split in two different places may
> not be the cleanest approach? One could argue they serve different
> purposes so perhaps it correct that we define them separately.  I am
> fine with either, but now I probably prefer the new enum to fixed
> function declaration mappings that Andres suggested. I agree it would be
> less error prone.

I'd really just entirely separate the two. I'd not include any knowledge
of this mechanism into smgr.h, and just make the expansion of paths
dispatch statically dispatched (potentially into md.c) to actually do
the work.  I'm not sure why sync.h would need any forward declarations
for that?

Greetings,

Andres Freund