On 14 May 2018 at 17:29, Amit Langote <langote_amit...@lab.ntt.co.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