From: Shawn Debnath <sdn@amazon.com>
To: Andres Freund <andres@anarazel.de>
Cc: Thomas Munro <thomas.munro@gmail.com>
Cc: Thomas Munro <thomas.munro@enterprisedb.com>
Cc: Dmitry Dolgov <9erthalion6@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: Refactoring the checkpointer's fsync request queue
Date: Fri, 22 Feb 2019 15:45:50 -0800
Message-ID: <20190222234549.GA7397@f01898859afd.ant.amazon.com> (raw)
In-Reply-To: <20190222230916.5mfsqn3oesd2nxiz@alap3.anarazel.de>
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>
> > 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)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: sdn@amazon.com, andres@anarazel.de, thomas.munro@gmail.com, thomas.munro@enterprisedb.com, 9erthalion6@gmail.com, robertmhaas@gmail.com
Subject: Re: Refactoring the checkpointer's fsync request queue
In-Reply-To: <20190222234549.GA7397@f01898859afd.ant.amazon.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox