Re: Incorrect comment in get_partition_dispatch_recurse

On 17 May 2018 at 02:51, Robert Haas <robertmhaas@gmail.com> wrote:
> On Mon, May 14, 2018 at 12:57 AM, David Rowley
> <david.rowley@2ndquadrant.com> wrote:
>> The "minus 1" part is incorrect. It simply just stores the 0-based
>> index of the item in the list. I was going to fix it by removing just
>> that part, but instead, I ended up rewriting the whole thing.
>
> 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.

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.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services