Re: move PartitionBoundInfo creation code

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