On 2018-Nov-07, Michael Paquier wrote: > On Wed, Nov 07, 2018 at 03:34:38PM +0900, Amit Langote wrote: > > On 2018/11/05 16:21, Michael Paquier wrote: > >> On Thu, Nov 01, 2018 at 01:03:00PM +0900, Amit Langote wrote: > >>> Done a few moments ago. :) > >> > >> From the file size this move is actually negative. From what I can see > >> partcache decreases to 400 lines, while partbounds increases to 3k > >> lines. > > > > Hmm, is that because partbounds.c contains more functionality? > > The move you are doing here makes sense, now refactoring is better if we > could avoid transforming a large file into a larger one and preserve > some more balance in the force.
I think the code as it's structured in Amit's patch as a whole makes sense -- partcache is smaller than partbounds, but why do we care? As long as each is a reasonably well defined module, it seems better. In the current situation we have a bit of a mess there. A 3k file is okay. We're not talking about bloating, say, xlog.c which is almost 400kb now. $ ls -lh src/backend/access/transam/xlog* -rw-r--r-- 1 alvherre alvherre 23K oct 4 11:40 src/backend/access/transam/xlogarchive.c -rw-r--r-- 1 alvherre alvherre 389K nov 7 11:49 src/backend/access/transam/xlog.c -rw-r--r-- 1 alvherre alvherre 21K nov 3 12:15 src/backend/access/transam/xlogfuncs.c -rw-r--r-- 1 alvherre alvherre 30K sep 3 12:58 src/backend/access/transam/xloginsert.c -rw-r--r-- 1 alvherre alvherre 40K sep 3 12:58 src/backend/access/transam/xlogreader.c -rw-r--r-- 1 alvherre alvherre 32K ago 5 21:34 src/backend/access/transam/xlogutils.c > > I think we can design the interface of partition_bounds_create such that > > it returns information needed to re-arrange OIDs to be in the canonical > > partition bound order, so the actual re-ordering of OIDs can be done by > > the caller itself. (It may not always be OIDs, could be something else > > like a struct to represent fake partitions.) This is interesting. I don't think the current interface is so bad that we can't make a few tweaks later; IOW let's focus on the current patch for now, and we can improve later. You (and David, and maybe someone else) already have half a dozen patches touching this code, and I bet some of these will have to be rebased over this one. > Thinking crazy, we could also create a subfolder partitioning/bounds/ > which includes three files with this refactoring:x > - range.c > - values.c > - hash.c > Then we keep partbounds.c which is the entry point used by partcache.c, > and partbounds.c relies on the stuff in each other sub-file when > building a strategy. This move could be interesting in the long term as > there are comparison routines and structures for each strategy if more > strategies are added. I'm not sure this is worthwhile. You'll create a bunch of independent translation units in exchange for ...? -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
