On 2018/05/17 3:28, David Rowley wrote:
> On 17 May 2018 at 02:51, Robert Haas <[email protected]> wrote:
>> I think that's clearer. Committed with a few tweaks that are
>> hopefully improvements.
>
> Thanks for committing. Although, I disagree with your tweak:
>
> + * 1-based index into the *pds list.
>
> I think that's making the same mistake as the last comment did. You
> think it's 1-based because the index is being set with list_length
> rather than list_length - 1, but it can do that simply because the
> item has not been added to the list yet.
>
> Nothing converts this index back to 0-based;
>
> RelationGetPartitionDispatchInfo builds the array from the list with:
>
> i = 0;
> foreach(lc, pdlist)
> {
> pd[i++] = lfirst(lc);
> }
>
> ExecFindPartition uses the pd array with:
>
> parent = pd[-parent->indexes[cur_index]];
>
> So if it was 1-based then we'd be off by one here.
That's right. Even those negative values in the pd->indexes are still
0-based, with the 0th entry being for the root table.
> Maybe we can clear up that confusion with
>
> + /*
> + * No need to subtract 1 to get the 0-based index as the item for this
> + * partitioned table has not been added to the list yet.
> + */
> pd->indexes[i] = -list_length(*pds);
>
> and just switch 1-based to 0-based in the new comment.
Or maybe, change the comment to say that even the negative indexes are
0-based like you pointed out, *but* instead of updating the comment like
you suggest above, change the other index value assignment statement to
not subtract 1 from the list_length by switching order with the
accompanying lappend; like this:
if (get_rel_relkind(partrelid) != RELKIND_PARTITIONED_TABLE)
{
+ pd->indexes[i] = list_length(*leaf_part_oids);
*leaf_part_oids = lappend_oid(*leaf_part_oids, partrelid);
- pd->indexes[i] = list_length(*leaf_part_oids) - 1;
}
else
{
Attached a patch.
Thanks,
Amit
diff --git a/src/backend/executor/execPartition.c
b/src/backend/executor/execPartition.c
index 75329b3624..da962c4375 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -975,7 +975,7 @@ get_partition_dispatch_recurse(Relation rel, Relation
parent,
* array element belongs to a leaf partition or a subpartitioned table.
* For leaf partitions we store the 0-based index into *leaf_part_oids,
* and for sub-partitioned tables we store a negative version of the
- * 1-based index into the *pds list. When searching, if we see a
negative
+ * 0-based index into the *pds list. When searching, if we see a
negative
* value, the search must continue in the corresponding sub-partition;
* otherwise, we've identified the correct partition.
*/
@@ -986,8 +986,8 @@ get_partition_dispatch_recurse(Relation rel, Relation
parent,
if (get_rel_relkind(partrelid) != RELKIND_PARTITIONED_TABLE)
{
+ pd->indexes[i] = list_length(*leaf_part_oids);
*leaf_part_oids = lappend_oid(*leaf_part_oids,
partrelid);
- pd->indexes[i] = list_length(*leaf_part_oids) - 1;
}
else
{
