Re: Incorrect comment in get_partition_dispatch_recurse

On 2018/05/14 20:29, David Rowley wrote:
> On 14 May 2018 at 17:29, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> 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.

Hmm, yes. I guess I had not bothered back then to confirm that the
pg_list.h Lists are in fact 0-based. That said, I wasn't thinking of the
physical implementation of lists when writing the comment, but probably of
a logical sequence, IOW, I had it all mixed up. Anyway, it's good that
we're getting rid of that misguided terminology and thank you for that.

Regards,
Amit