On Wed, Nov 07, 2018 at 12:04:27PM -0300, Alvaro Herrera wrote:
> On 2018-Nov-07, Michael Paquier wrote:
> 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.
Yeah, I am actually quite a fan of having the mapping being returned by
partition_bound_create and just let the caller handle the OID
assignment. That's neater. I think that the patch could gain a tad
more in clarity by splitting all sub-processing for each strategy into a
single routine though. The presented switch/case is a bit hard to
follow in the last patch set, and harder to review in details.
>> 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 ...?
That's the part about going crazy and wild on this refactoring. Not all
crazy ideas are worth doing, still I like mentioning all possibilities
so as we miss nothing in the review process.
--
Michael
