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
signature.asc
Description: PGP signature