Re: move PartitionBoundInfo creation code
Michael Paquier <michael@paquier.xyz>
From: Michael Paquier <michael@paquier.xyz>
To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>, Pg Hackers <pgsql-hackers@postgresql.org>
Date: 2018-11-12T13:55:41Z
Lists: pgsql-hackers
Attachments
- partition-bound-refactor.patch (text/x-diff) patch
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
Commits
-
Refactor code creating PartitionBoundInfo
- b52b7dc250bb 12.0 landed