Thanks Amit for the patch. I am still reviewing it, but meanwhile below are a few comments so far ...
On 8 September 2017 at 15:53, Amit Langote <langote_amit...@lab.ntt.co.jp> wrote: > [PATCH 2/2] Make RelationGetPartitionDispatch expansion order > depth-first > > This is so as it matches what the planner is doing with partitioning > inheritance expansion. Matching with planner order helps because > it helps ease matching the executor's per-partition objects with > planner-created per-partition nodes. > > + next_parted_idx += (list_length(*pds) - next_parted_idx - 1); I think this can be replaced just by : + next_parted_idx = list_length(*pds) - 1; Or, how about removing this variable next_parted_idx altogether ? Instead, we can just do this : pd->indexes[i] = -(1 + list_length(*pds)); If that is not possible, I may be missing something. ----------- + next_leaf_idx += (list_length(*leaf_part_oids) - next_leaf_idx); Didn't understand why next_leaf_idx needs to be updated in case when the current partrelid is partitioned. I think it should be incremented only for leaf partitions, no ? Or for that matter, again, how about removing the variable 'next_leaf_idx' and doing this : *leaf_part_oids = lappend_oid(*leaf_part_oids, partrelid); pd->indexes[i] = list_length(*leaf_part_oids) - 1; ----------- * For every partitioned table in the tree, starting with the root * partitioned table, add its relcache entry to parted_rels, while also * queuing its partitions (in the order in which they appear in the * partition descriptor) to be looked at later in the same loop. This is * a bit tricky but works because the foreach() macro doesn't fetch the * next list element until the bottom of the loop. I think the above comment needs to be modified with something explaining the relevant changed code. For e.g. there is no parted_rels, and the "tricky" part was there earlier because of the list being iterated and at the same time being appended. ------------ I couldn't see the existing comments like "Indexes corresponding to the internal partitions are multiplied by" anywhere in the patch. I think those comments are still valid, and important. -- Thanks, -Amit Khandekar EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
