Date: Fri, 22 Feb 2019 15:45:50 -0800
From: Shawn Debnath <sdn@amazon.com>
To: Andres Freund <andres@anarazel.de>
CC: Thomas Munro <thomas.munro@gmail.com>,
        Thomas Munro
	<thomas.munro@enterprisedb.com>,
        Dmitry Dolgov <9erthalion6@gmail.com>,
        Robert Haas <robertmhaas@gmail.com>,
        PostgreSQL-development
	<pgsql-hackers@postgresql.org>
Subject: Re: Refactoring the checkpointer's fsync request queue
Message-ID: <20190222234549.GA7397@f01898859afd.ant.amazon.com>
References: <20190216193905.GA53174@f01898859afd.ant.amazon.com>
 <20190220232739.GA8280@f01898859afd.ant.amazon.com>
 <20190220235026.2pxdcmwtijijax32@alap3.anarazel.de>
 <CA+hUKGK8yRjDc1ED8bmZavm=g8TYmZKOzQF3QUx_HhOUCSzkZg@mail.gmail.com>
 <20190222181857.GA77583@f01898859afd.ant.amazon.com>
 <20190222221547.kgobq55dk3dugpv5@alap3.anarazel.de>
 <CA+hUKG+7q=1wR+RuNUb1TSST-SjG4sr-_yfyny2MnsQSyY7x_Q@mail.gmail.com>
 <20190222224853.ahvm3bf52zqr7k7h@alap3.anarazel.de>
 <CA+hUKG+i5+0Grt9O9gtv1YHASP6R3rfb8074or0j=fFoY6YvTw@mail.gmail.com>
 <20190222230916.5mfsqn3oesd2nxiz@alap3.anarazel.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20190222230916.5mfsqn3oesd2nxiz@alap3.anarazel.de>
User-Agent: Mutt/1.11.3 (2019-02-01)
Precedence: Bulk
Precedence: bulk

> > 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.

> > > > 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.


-- 
Shawn Debnath
Amazon Web Services (AWS)