Re: Incorrect comment in get_partition_dispatch_recurse

On 14 May 2018 at 17:29, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> On 2018/05/14 13:57, David Rowley wrote:
>> I noticed that a comment in get_partition_dispatch_recurse claims that:
>>
>> "it contains the
>> * leaf partition's position in the global list *leaf_part_oids minus 1"
>>
>> The "minus 1" part is incorrect. It simply just stores the 0-based
>> index of the item in the list.
>
> Hmm, while I agree that simply calling it "0-based index" might be better
> for readers, what's there now doesn't sound incorrect to me; in the
> adjacent code:
>
> if (get_rel_relkind(partrelid) != RELKIND_PARTITIONED_TABLE)
> {
> *leaf_part_oids = lappend_oid(*leaf_part_oids, partrelid);
> pd->indexes[i] = list_length(*leaf_part_oids) - 1;
> }
>
> If I call the value of list_length after adding an OID to the list the
> OID's position in the list, we're storing into the indexes array exactly
> what the existing comment says it is. Now, literally describing the code
> in the adjacent comment is not a great documentation style, so I'm open to
> revising it like your patch does. :)

Thanks for looking.

I wouldn't have complained if list_nth() accepted a 1-based index, but
it does not. So, indexes[] does not store the "position in the global
list *leaf_part_oids minus 1", it just stores the position in the
list.

I imagine it's only written this way due to the way you're obtaining
the index using list_length(*leaf_part_oids) - 1, but the fact you had
to subtract 1 there does not make it "position minus 1". That's just
how you get the position of the list item in a List.

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