Re: move PartitionBoundInfo creation code

Hi,

Thank your for taking a look.

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?

> There are a couple of things that this patch is doing:
> 1) Move the functions comparing two bounds into partbounds.c.
> 2) Remove the chunk in charge of building PartitionBoundData into
> partbounds.c for each method: list, hash and values.
>
> From what I can see, this patch brings actually more confusion by doing
> more things than just building all the PartitionBound structures as it
> fills in each structure and then builds a mapping which is used to save
> each partition OID into the correct mapping position. Wouldn't it move
> on with a logic without this mapping so as the partition OIDs are
> directly part of PartitionBound? It looks wrong to me to have
> build_partition_boundinfo create not only partdesc->boundinfo but also
> partdesc->oids, and the new routine is here to fill in data for the
> former, not the latter.

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.)

The canonical partition bound order is basically partition bounds sorted
in ascending order, or in some manner as defined by the qsort_partition_*
functions of individual partitioning methods. We need to map the new
canonical positions to the older ones by generating a map, which it only
makes sense to do in partition_bounds_create.

> The first phase building the bounds should switch to a switch/case like
> the second phase.

That bothered me too, so I looked at whether it'd be a good idea to have
just one switch () block and put all the code for different partitioning
methods into it. The resulting code seems more readable to me as one
doesn't have to shuffle between looking at the first block that generates
Datums from PartitionBoundSpecs and the second block that assigns them to
the Datum array in PartitionBoundInfo, for each partitioning method
respectively.

> PartitionHashBound & friends can become structures local to
> partbounds.c as they are used only there.

Ah, I missed that; moved in.

> To be more consistent with all the other routines, like
> partition_bounds_equal/copy, wouldn't it be better to call the new
> routine partition_bounds_build or partition_bounds_create?

Agree about naming consistency; I went with partition_bounds_create.

I've broken this down this into 3 patches for ease of review:

1. Local re-arrangement of the code in RelationBuildPartitionDesc such
that the code that creates PartitionBoundInfo is clearly separated. I've
also applied some of the comments above to that piece of code such as
unifying all of that code in one switch() block. I also found an
opportunity to simplify the code that maps canonical partition indexes to
original partition indexes.

2. Re-arrangement local to partcache.c that moves out the separated code
into partition_bounds_create() with the interface mentioned above.

3. Move partition_bounds_create to partbounds.c and make some functions
that are not needed outside partbounds.c static to that file.

Thanks,
Amit