Re: move PartitionBoundInfo creation code

Hi,

On 2018/11/12 22:55, Michael Paquier wrote:
> On Thu, Nov 08, 2018 at 03:11:35PM +0900, Amit Langote wrote:
>> And here is the new version. The break down into smaller local
>> functions for different partitioning methods is in patch 0002.
>
> Okay, here we go.

Thank you for reviewing.

> I have spent a couple of hours on this patch,
> checking the consistency of the new code and the new code, and the main
> complain I have about the last version is that the code in charge of
> building the mapping was made less robust.
> So my notes are:
> - The initialization of the mapping should happen in
> partition_bounds_create() before going through each routine. The
> proposed patch was only doing the initialization for list bounds, using
> an extra layer to track the canonical ordering of indexes associated
> with a partition (listpart_canon_idx), and that's something that the
> mapping can perfectly do, keeping the code more straight-forward (at
> least it seems so to me) with the way null_index and default_index are
> assigned.

I looked at this and the new implementation seems safe and is a bit faster
due to removing listpart_canon_idx stuff.

> - Some noise diffs were present, sometimes the indentation being wrong.
> - Asserts could be used instead of elog(ERROR) if the strategy is not
> the expected one. That looks cleaner to me.

Hmm, Assert or elog is your call, but I'd learned that we prefer elog when
checking a value read from the disk?

> The result is perhaps less smart than what you did, but that's more
> robust. It may make sense to change the way the mapping is built but
> I would suggest to do so after the actual refactoring. Attached is what
> I finish with, and I have spent quite some time making sure that
> all the new logic remains consistent with the previous one.

Thank you, the new logic seems sane to me.

I noticed a few things while going over the new patch.

I'd accidentally ended up changing the mapping elements to mean that they
map canonical indexes of partitions to their original indexes, but your
rewrite has restored the original meaning, so the following comment that I
wrote is obsolete:

+ * Upon return from this function, *mapping is set to an array of
+ * list_length(boundspecs) elements, each of which maps the canonical
+ * index of a given partition to its 0-based position in the original list.

It should be: "each of which maps the original index of a partition to its
canonical index."

+ /*
+ * For each partitioning method, we first convert the partition bounds
+ * from their parser node representation to the internal representation,
+ * along with any additional preprocessing (such performing de-duplication
+ * on range bounds). For each bound, we remember its partition's position
+ * (0-based) in the original list, so that we can add it to the *mapping
+ * array.
+ *
+ * Resulting bound datums are then added to the 'datums' array in
+ * PartitionBoundInfo. For each datum added, an integer indicating the
+ * canonical partition index is added to the 'indexes' array.
+ */

Maybe we can slightly rearrange this comment (along with fixing typos and
obsolete facts) as:

/*
* For each partitioning method, we first convert the partition bounds f
* from their parser node representation to the internal representation,
* along with any additional preprocessing (such as de-duplicating range
* bounds). Resulting bound datums are then added to the 'datums' array
* in PartitionBoundInfo. For each datum added, an integer indicating the
* canonical partition index is added to the 'indexes' array.
*
* For each bound, we remember its partition's position (0-based) in the
* original list to later map it to the canonical index.
*/

I'd proposed to rewrite the following comments in create_list_bounds, but
maybe you thought it's related to listmap_canon_idx stuff which no longer
exists.

+ /*
+ * Copy values. Indexes of individual values are mapped to canonical
+ * values so that they match for any two list partitioned tables with same
+ * number of partitions and same lists per partition. One way to
+ * canonicalize is to assign the index in all_values[] of the smallest
+ * value of each partition, as the index of all of the partition's values.
+ */

Here's what I'd proposed (fixing typos and removing the sentence that
mentioned listmap_canon_idx).

/*
* Copy values. Canonical indexes are values ranging from 0 to nparts - 1
* assigned to each partition such that all datums of a given partition
* receive the same value. The value for a given partition is the index
* of that partition's smallest datum in the all_values[] array.
*/

Similarly, for:

+ /*
+ * If null-accepting partition has no mapped index yet, assign one. This
+ * could happen if such partition accepts only null and hence not covered
+ * in the above loop which only handled non-null values.
+ */

I'd proposed:

/*
* Set the canonical value for null_index, if any.
*
* It's possible that the null-accepting partition has not been
* assigned an index yet, which could happen if such partition
* accepts only null and hence not handled in the above loop
* which only looked at non-null values.
*/

For:

+ /* Assign mapped index for the default partition. */

I'd proposed:

/* Set the canonical value for default_index, if any. */

Thanks,
Amit