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