On Tue, Nov 13, 2018 at 10:34:50AM +0900, Amit Langote wrote:
> Hmm, Assert or elog is your call, but I'd learned that we prefer elog when
> checking a value read from the disk?
Let's keep elog() then, which is consistent with the previous code. If
there is any meaning in switching to an assert(), it could always be
changed later, if that proves necessary.
> Thank you, the new logic seems sane to me.
>
> I noticed a few things while going over the new patch.
Thanks for going through it.
> 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."
Indeed. Good point. I have missed that bit.
> 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.
> */
OK, that looks cleaner.
> 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.
> */
No issues with that. So canonical index is used as a more common term,
which seems fine.
> I'd proposed:
>
> /* Set the canonical value for default_index, if any. */
And this one either, using a conditional at the end is more adapted
anyway. And there are two occurences of that.
Attached is an updated patch. Perhaps you are spotting something else?
--
Michael